Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
htmlchat | 3208a14 | Sep 18 2025, 08:04 AM |
WalkthroughAdds a Vite-built frontend under Build/ (full chat UI, PMs, uploads, search, notifications, moderation), replaces the legacy single-file client with the Build app, updates GitHub Pages workflow to build & publish Build/dist, and extends the Worker backend with routing, uploads, file serving, CORS, moderation, and private messaging. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant FE as HTMLChat (Build)
participant BE as Worker API
participant DO as ChatRoom (Durable)
rect rgb(240,248,255)
note right of FE: App load & initial fetch
U->>FE: Open app
FE->>BE: GET /chat/{room}
BE->>DO: Forward request
DO-->>BE: Messages + users + moderator data
BE-->>FE: JSON response
FE->>FE: Render messages/users
end
sequenceDiagram
autonumber
participant U as User
participant FE as HTMLChat
participant BE as Worker API
participant FB as FILE_BUCKET
rect rgb(255,250,240)
note right of FE: File upload flow
U->>FE: Choose/Drop file(s)
FE->>BE: POST /upload (FormData)
BE->>BE: Validate size/type
alt Stored in bucket
BE->>FB: PUT object
FB-->>BE: Stored, URL
BE-->>FE: {url, filename, ...}
else Fallback (inline)
BE-->>FE: {dataURL,...}
end
FE->>BE: POST /chat/{room} (FILE message)
BE-->>FE: OK
FE->>FE: Render attachment
end
sequenceDiagram
autonumber
participant M as Moderator
participant FE as HTMLChat
participant BE as Worker API
participant DO as ChatRoom
rect rgb(245,245,255)
note over FE,DO: Moderation action (ban/kick/delete)
M->>FE: Trigger action via context menu
FE->>BE: POST /mod/{room} {action,target,...}
BE->>DO: Authorize and apply
alt Authorized
DO-->>BE: Success
BE-->>FE: OK
FE->>BE: GET /chat/{room}
BE-->>FE: Updated data
else Forbidden
DO-->>BE: 403
BE-->>FE: Error
end
end
sequenceDiagram
autonumber
participant A as User A
participant FE as HTMLChat
participant BE as Worker API
participant PM as PM Durable
rect rgb(250,245,255)
note right of FE: Private messaging
A->>FE: Open PM window
FE->>BE: GET /pm/{conversation}
BE->>PM: Fetch history
PM-->>BE: Messages
BE-->>FE: History
A->>FE: Send PM
FE->>BE: POST /pm/{conversation} {to,text}
BE->>PM: Append message
PM-->>BE: OK
BE-->>FE: OK
FE->>FE: Refresh PM view
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
|
i'm calling it that i'll be yelled at for either XSS or the Open Server |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Nitpick comments (42)
Build/package.json (1)
1-32: Mark package private and move Vite to devDependencies.Prevents accidental publish and avoids shipping dev-only deps.
Apply this diff:
{ "name": "htmlchat", "version": "0.2.0", "description": "A retro-styled, browser-only chat client.", - "main": "index.js", + "main": "index.js", + "private": true, "scripts": { "dev": "vite", "build": "vite build", "preview": "vite preview" }, @@ "homepage": "https://github.com/HTMLToolkit/HTMLChat#readme", - "dependencies": { + "dependencies": { "dompurify": "^3.2.6", "lucide": "^0.544.0", - "marked": "^16.3.0", - "vite": "^7.1.5" - } + "marked": "^16.3.0" + }, + "devDependencies": { + "vite": "^7.1.5" + } }TODO.md (1)
1-26: Add trailing newline.Keeps tooling happy and avoids “No newline at end of file” diffs.
Apply this diff:
- - [X] Better UI + - [X] Better UI +README.md (1)
20-26: Fix markdownlint issues (dash vs asterisk, tabs, indent).Swap dashes to asterisks for the indicated lists and replace tabs with two spaces under “Running it”.
Apply this diff:
- - Export chat logs as JSON (plus a handy Reload button next to Export) - - File uploads (images/docs) with previews - - Replies (click to reply, threaded context) - - Search (fast, non-blocking) - - Moderator tools (delete/ban) - - Settings modal (desktop notifications + sounds toggles) - - Lucide icons via npm (no CDN, crisp SVGs) + * Export chat logs as JSON (plus a handy Reload button next to Export) + * File uploads (images/docs) with previews + * Replies (click to reply, threaded context) + * Search (fast, non-blocking) + * Moderator tools (delete/ban) + * Settings modal (desktop notifications + sounds toggles) + * Lucide icons via npm (no CDN, crisp SVGs) @@ -- Frontend lives in `Build/` and uses Vite. - - Dev: `cd Build && npm install && npm run dev` - - Build: `cd Build && npm install && npm run build` +- Frontend lives in `Build/` and uses Vite. + - Dev: `cd Build && npm ci && npm run dev` + - Build: `cd Build && npm ci && npm run build`Also applies to: 42-45
Build/src/styles.css (2)
69-71: Remove duplicate CSS blocks (confusing overrides).
.header-btn.muted,.permission-status/#permission-status-text, and.small-btnare defined twice. Keep one source of truth.Apply this diff to drop the later duplicates:
-/* Sound toggle states */ -.header-btn.muted { - opacity: 0.5; -} - -/* Permission Status */ -.permission-status { - display: flex; - align-items: center; - gap: 8px; - font-size: 14px; -} - -#permission-status-text { - font-weight: 500; - padding: 2px 8px; - border-radius: 4px; - font-size: 12px; -} - -#permission-status-text.granted { - background-color: #d4edda; - color: #155724; -} - -#permission-status-text.denied { - background-color: #f8d7da; - color: #721c24; -} - -#permission-status-text.default { - background-color: #fff3cd; - color: #856404; -} - -.small-btn { - padding: 4px 8px; - font-size: 12px; - background: #007bff; - color: white; - border: none; - border-radius: 4px; - cursor: pointer; -} - -.small-btn:hover { - background: #0056b3; -}Also applies to: 855-857, 931-949, 1021-1049, 950-960, 1051-1063
1-1063: Add reduced‑motion support (a11y).Respect users’ motion preferences for transitions/animations.
Apply this diff:
+@media (prefers-reduced-motion: reduce) { + * { + animation-duration: 0.01ms !important; + animation-iteration-count: 1 !important; + transition-duration: 0.01ms !important; + scroll-behavior: auto !important; + } +}.github/workflows/static.yml (1)
26-46: CI: build on PRs too; keep deploy on main.Add a PR build job (no deploy) to catch breaks early.
Suggested snippet (separate job):
on: push: branches: ["main"] pull_request: branches: ["main"] jobs: build: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: node-version: '20' cache: npm cache-dependency-path: Build/package-lock.json - name: Install working-directory: Build run: npm ci - name: Build working-directory: Build run: npm run build deploy: if: github.ref == 'refs/heads/main' needs: build runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 # ... keep existing deploy stepsBuild/src/messageRenderer.js (5)
96-101: Don’t allow arbitrary style attributes from user content.Allowing style increases XSS surface. Let CSS handle presentation; drop style from ALLOWED_ATTR.
- ALLOWED_ATTR: ['href','src','alt','title','target','style'] + ALLOWED_ATTR: ['href','src','alt','title','target']
158-160: Duplicate class append.system class is appended twice.
- if (message.system) messageClass += ' system'; - if (message.system) messageClass += ' system'; + if (message.system) messageClass += ' system';
83-89: Unify reply header regex.processText requires numeric id; renderMessages accepts any. Pick one shape (prefer the latter) so replies parse consistently.
- const replyMatch = text.match(/^@reply:(\d+):([^:]+):\s*(.*)/); + const replyMatch = text.match(/^@reply:([^:]+):([^:]+):\s*(.*)/);Also applies to: 124-133
42-55: Remove debug logging.Leaky console logs for user content/ids.
- console.log('Reply parsed:', { messageId, replyUser, messageText }); // Debug + // (debug removed)Also applies to: 132-133
104-104: Safari lookbehind compatibility.Negative lookbehind can still bite on older Safari. Consider a DOM walker or a safer URL linker (e.g., marked’s linkify plugin) instead of regex.
Build/src/contextMenu.js (4)
10-16: Guard when #context-menu is missing.Avoid NPEs if the menu element isn’t present.
setupEventListeners() { + if (!this.menu) { + console.warn('Context menu element #context-menu not found'); + return; + }
19-24: Use event delegation with closest() for clicks.Clicking icon/text inside a menu item won’t bubble a dataset on target.
- this.menu.addEventListener('click', (e) => { - const action = e.target.dataset.action; + this.menu.addEventListener('click', (e) => { + const el = e.target.closest('[data-action]'); + const action = el && el.dataset.action; if (action && this.currentMessage) { this.handleAction(action); } });
35-76: Position reliably: measure after making the menu renderable.getBoundingClientRect on display:none returns 0,0. Briefly show invisibly to measure.
show(event, messageElement) { event.preventDefault(); @@ - const rect = this.menu.getBoundingClientRect(); + this.menu.style.visibility = 'hidden'; + this.menu.style.display = 'block'; + const rect = this.menu.getBoundingClientRect(); @@ - this.menu.style.display = 'block'; + this.menu.style.visibility = 'visible';
237-251: Validate ban duration and handle “0” correctly.Current logic treats "0" as empty and allows NaN through.
- const reason = prompt(`Ban ${this.currentMessage.user}? Enter reason (optional):`); - if (reason !== null) { - const duration = prompt('Ban duration (minutes, or leave empty for permanent):'); + const reason = prompt(`Ban ${this.currentMessage.user}? Enter reason (optional):`); + if (reason !== null) { + const durationStr = prompt('Ban duration (minutes, or leave empty for permanent):'); + let duration = null; + if (durationStr !== null && durationStr.trim() !== '') { + const parsed = parseInt(durationStr, 10); + if (Number.isNaN(parsed) || parsed < 0) { + alert('Invalid duration'); + return; + } + duration = parsed; + } @@ - duration: duration ? parseInt(duration) : null + durationBuild/src/fileUpload.js (1)
118-145: Optional: add input accept attribute + dedupe by lastModified.Small UX + correctness wins.
Would you like a follow-up diff that sets fileInput.accept to allowed types and also checks lastModified when deduping?
Build/src/privateMessages.js (1)
7-9: Ensure PM container exists.Guard against null to avoid runtime errors on construction.
Add:
if (!this.windowContainer) console.warn('#pm-windows not found');Build/src/soundManager.js (1)
50-55: Trim debug log.Reduce console noise; UI already reflects state.
toggleSounds() { this.soundsEnabled = !this.soundsEnabled; this.saveSetting('sounds_enabled', this.soundsEnabled); - console.log('Sound manager toggled to:', this.soundsEnabled); // Debug return this.soundsEnabled; }Worker/src/index.js (4)
10-20: CORS preflight: echo request headers and add max-age.Preflight currently allows only Content-Type. Echoing Access-Control-Request-Headers and adding Access-Control-Max-Age improves compatibility and reduces OPTIONS load.
Apply:
if (request.method === 'OPTIONS') { return new Response('', { status: 204, headers: { - 'Access-Control-Allow-Origin': '*', - 'Access-Control-Allow-Methods': 'GET, POST, PUT, DELETE, OPTIONS', - 'Access-Control-Allow-Headers': 'Content-Type' + 'Access-Control-Allow-Origin': '*', + 'Access-Control-Allow-Methods': 'GET, POST, PUT, DELETE, OPTIONS', + 'Access-Control-Allow-Headers': request.headers.get('Access-Control-Request-Headers') || 'Content-Type', + 'Access-Control-Max-Age': '86400' } }); }
51-54: /files 404 lacks CORS headers.Without ACAO on 404, browsers may block error handling.
- if (!object) { - return new Response('File not found', { status: 404 }); - } + if (!object) { + return new Response('File not found', { + status: 404, + headers: { + 'Content-Type': 'text/plain', + 'Access-Control-Allow-Origin': '*' + } + }); + }
73-85: Version mismatch: health says 2.0.0, UI shows v0.2.0.Align the reported version or source it from a single env/config.
- version: '2.0.0' + version: env.VERSION || '0.2.0'And inject VERSION during build/deploy.
166-177: Set contentDisposition during R2 put.Improves downstream serving and consistent filenames; pairs with nosniff.
- await env.FILE_BUCKET.put(filename, file.stream(), { - httpMetadata: { - contentType: file.type - }, + await env.FILE_BUCKET.put(filename, file.stream(), { + httpMetadata: { + contentType: file.type, + contentDisposition: `inline; filename="${sanitizedName}"` + },Worker/src/chatRoom.js (6)
25-29: Avoid setInterval in Durable Objects; use alarms.Timers may not fire when the DO is idle and keep the instance alive unnecessarily.
- this.cleanupTimer = setInterval(() => this.cleanupUsers(), 30000); - - // Initialize moderators and banned users - this.initializeModerationData(); + // Initialize and schedule periodic cleanup via alarms + this.initializeModerationData(); + this.state.storage.getAlarm().then(alarm => { + if (!alarm) this.state.storage.setAlarm(Date.now() + 60 * 1000); + });Add an alarm handler:
export default class ChatRoom { + async alarm() { + await this.cleanupUsers(); + await this.cleanupExpiredBans(); + await this.cleanupExpiredKicks(); + await this.state.storage.setAlarm(Date.now() + 60 * 1000); + }
221-224: Normalize user_messages key to avoid “User” vs “user” bypass.Case differences create parallel histories and weaken spam checks.
- const userMessages = await this.state.storage.get(`user_messages:${user}`) || []; + const userKey = String(user).toLowerCase(); + const userMessages = await this.state.storage.get(`user_messages:${userKey}`) || []; @@ - await this.state.storage.put(`user_messages:${user}`, recentMessages.slice(-10)); // Keep last 10 + await this.state.storage.put(`user_messages:${userKey}`, recentMessages.slice(-10)); // Keep last 10
306-315: Presence PUT ignores kicked error. Return 403 when kicked.Currently you return users even if updateUserPresence rejected with an error.
if (request.method === 'PUT') { - await this.updateUserPresence(room, user); + const res = await this.updateUserPresence(room, user); + if (res?.error) return textResponse(res.error, 403); const users = await this.getUsers(room); return jsonResponse({ users, userCount: users.length }); }
509-512: Case-insensitive removal from presence map on kick.Presence keys are stored with original case; delete may no-op if cases differ.
- delete users[targetUser]; + const keyToDelete = Object.keys(users).find(u => u.toLowerCase() === targetUser.toLowerCase()); + if (keyToDelete) delete users[keyToDelete];
517-523: Message text says “banned” for a kick.Use “kicked” to avoid confusion.
- text: `${targetUser} was kicked by ${moderator}${reason ? ` (${reason})` : ''} - banned for 5 minutes`, + text: `${targetUser} was kicked by ${moderator}${reason ? ` (${reason})` : ''} - kicked for 5 minutes`,
348-357: Avoid logging message contents in production.Logs may leak PII; gate logs behind an env flag or remove.
- console.log(`Delete request: room=${room}, messageId=${messageId}, user=${user}`); // Debug - console.log(`Found message at index: ${messageIndex}`); // Debug + if (this.env?.DEBUG) console.log(`Delete request: room=${room}, messageId=${messageId}, user=${user}`); + if (this.env?.DEBUG) console.log(`Found message at index: ${messageIndex}`); @@ - console.log(`Message to delete:`, message); // Debug + if (this.env?.DEBUG) console.log(`Message to delete:`, { id: message.id, user: message.user });Build/index.html (2)
326-331: CDN script contradicts “no CDN” goal and adds prod risk.Load eruda only in dev via bundler; remove the CDN include.
- <script src="https://cdn.jsdelivr.net/npm/eruda"></script> - <script> - eruda.init(); - </script> + <!-- Dev-only debug should be imported via Vite in src/main.js (e.g., if (import.meta.env.DEV) import('eruda').then(m=>m.default.init())) -->
175-179: Align accept= with server allowlist to reduce UX errors.Browser may allow selection the server will reject (e.g., audio/* includes unsupported types).
- accept="image/*,audio/*,.pdf,.doc,.docx,.txt" + accept="image/jpeg,image/png,image/gif,image/webp,audio/mpeg,audio/wav,audio/ogg,application/pdf,text/plain,application/msword,application/vnd.openxmlformats-officedocument.wordprocessingml.document"Build/src/search.js (4)
95-98: Storage key mismatch may hide messages from search.Elsewhere export uses htmlchat_messages; here you read htmlchat_.
- const messages = this.app.loadFromStorage(`htmlchat_${currentRoom}`) || []; + const messages = + this.app.loadFromStorage(`htmlchat_${currentRoom}`) || + this.app.loadFromStorage('htmlchat_messages') || + [];Please standardize keys across modules.
155-159: Prefer real message id when available.Deriving from time+index is brittle and may not match DOM ids.
- const messageId = `msg-${msg.time}-${index}`; + const messageId = msg.id ? String(msg.id) : `msg-${msg.time}-${index}`;Also update jumpToMessage to try data-message-id matching msg.id first.
106-114: Guard against missing fields.Optional chaining prevents crashes on system/non-text messages.
- const textMatch = msg.text.toLowerCase().includes(query) || - msg.user.toLowerCase().includes(query); + const textMatch = (msg.text?.toLowerCase?.() || '').includes(query) || + (msg.user?.toLowerCase?.() || '').includes(query);
205-225: Avoid fixed 500ms render wait.Prefer awaiting a render completion hook or requestAnimationFrame loops for a short retry window.
- this.app.fetchMessages(true).then(() => { - setTimeout(() => { + this.app.fetchMessages(true).then(async () => { + for (let i = 0; i < 5; i++) { // retry for a few frames + await new Promise(r => requestAnimationFrame(r)); // Try to find by messageId again let foundMessage = document.getElementById(messageId); if (!foundMessage) { foundMessage = document.querySelector(`[data-message-id="${messageId}"]`); } if (foundMessage) { this.app.messageRenderer.highlightMessage(foundMessage.id); - } else { - // Fallback: try to find by timestamp - const messages = this.app.loadFromStorage(`htmlchat_${this.app.elements.roomSelect.value}`) || []; - const messageIndex = messages.findIndex(msg => msg.time === timestamp); - if (messageIndex !== -1) { - const generatedId = `msg-${timestamp}-${messageIndex}`; - this.app.messageRenderer.highlightMessage(generatedId); - } - } - }, 500); + return; + } + } + // Fallback by timestamp if still not found + const messages = this.app.loadFromStorage(`htmlchat_${this.app.elements.roomSelect.value}`) || []; + const messageIndex = messages.findIndex(msg => msg.time === timestamp); + if (messageIndex !== -1) { + const generatedId = `msg-${timestamp}-${messageIndex}`; + this.app.messageRenderer.highlightMessage(generatedId); + } });Build/src/moderatorTools.js (3)
1-6: Client-side mod state is not authoritative.LocalStorage-based moderators/bans are trivially editable and can drift from server state.
- Treat the Worker (chatRoom.js) as source of truth; reflect its moderators/bans via authenticated API calls.
- Use this class as a thin cache/UI helper, not as the authority. Sync on load and on change, invalidate on errors.
42-56: Normalize moderator entries and persist through server API.Ensure adds/removes also call the backend (e.g., /mod endpoints) with proper auth, then update local cache on success.
Would you like a small client helper to wrap POST /mod {action} with x-admin-key and reconcile local state?
154-189: Duplicate spam/banned-word checks vs server; define single policy.Divergent client/server checks lead to inconsistent UX. Push rules to server and reuse results client-side.
Build/src/notifications.js (1)
99-124: Don’t wipe all classes on permission status elementResetting
classNamecan remove unrelated classes. Remove only what you add.Apply this diff:
- if (statusText) { - statusText.className = ''; + if (statusText) { + statusText.classList.remove('granted', 'denied', 'default');Build/src/main.js (4)
266-275: De‑dupe activity tracking listenersYou register the same listeners twice (here and in
setupActivityTracking). Keep one.Apply this diff to remove the duplicate block in
setupEventListeners:- // Activity tracking - ["click", "keypress", "scroll", "mousemove"].forEach((event) => { - document.addEventListener( - event, - () => { - this.lastActivity = Date.now(); - }, - { passive: true } - ); - });Also applies to: 297-308
195-227: Guard missing sound toggle element to avoid NPEIf
#sound-toggleis absent,querySelectorcalls will crash.Apply this diff:
const soundsEnabled = this.soundManager.isSoundEnabled(); const soundToggle = this.elements.soundToggle; - // Wait a bit for icons to initialize, then set the state - setTimeout(() => { + // Wait a bit for icons to initialize, then set the state + if (!soundToggle) return; + setTimeout(() => { const soundOnIcon = soundToggle.querySelector('.sound-on-icon'); const soundOffIcon = soundToggle.querySelector('.sound-off-icon'); - - console.log("Initial sound state:", soundsEnabled); // Debug - console.log("Initial icons found:", { soundOnIcon, soundOffIcon }); // Debug
203-205: Remove debug logsStrip noisy console logs before release.
Apply this diff:
- console.log("Initial sound state:", soundsEnabled); // Debug - console.log("Initial icons found:", { soundOnIcon, soundOffIcon }); // Debug @@ - console.log("Received data:", data); @@ - console.log("Sound toggle clicked, enabled:", isEnabled); // Debug @@ - console.log("Found icons:", { soundOnIcon, soundOffIcon }); // Debug @@ - console.log("Showing sound ON icon"); // Debug @@ - console.log("Showing sound OFF icon"); // DebugAlso applies to: 380-381, 625-656
361-407: Optional: unique notification tags per messageIf you want multiple notifications (not one replacing another), pass unique tags or let
NotificationManagerset none by default (as suggested). Otherwise only the last message may be visible in the OS tray.No code change required here if you accept the
NotificationManagerchange to drop the default tag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
Build/package-lock.jsonis excluded by!**/package-lock.jsonBuild/public/icons/icon-128x128.pngis excluded by!**/*.pngBuild/public/icons/icon-144x144.pngis excluded by!**/*.pngBuild/public/icons/icon-152x152.pngis excluded by!**/*.pngBuild/public/icons/icon-192x192.pngis excluded by!**/*.pngBuild/public/icons/icon-256x256.pngis excluded by!**/*.pngBuild/public/icons/icon-384x384.pngis excluded by!**/*.pngBuild/public/icons/icon-48x48.pngis excluded by!**/*.pngBuild/public/icons/icon-512x512.pngis excluded by!**/*.pngBuild/public/icons/icon-72x72.pngis excluded by!**/*.pngBuild/public/icons/icon-96x96.pngis excluded by!**/*.png
📒 Files selected for processing (21)
.github/workflows/static.yml(2 hunks)Build/.gitignore(1 hunks)Build/index.html(1 hunks)Build/package.json(1 hunks)Build/src/contextMenu.js(1 hunks)Build/src/fileUpload.js(1 hunks)Build/src/main.js(1 hunks)Build/src/messageRenderer.js(1 hunks)Build/src/moderatorTools.js(1 hunks)Build/src/notifications.js(1 hunks)Build/src/privateMessages.js(1 hunks)Build/src/search.js(1 hunks)Build/src/soundManager.js(1 hunks)Build/src/styles.css(1 hunks)Build/vite.config.js(1 hunks)README.md(3 hunks)TODO.md(1 hunks)Worker/src/chatRoom.js(6 hunks)Worker/src/index.js(1 hunks)Worker/wrangler.toml(1 hunks)index.html(0 hunks)
💤 Files with no reviewable changes (1)
- index.html
🧰 Additional context used
🧬 Code graph analysis (7)
Build/src/contextMenu.js (1)
Worker/src/index.js (1)
room(109-109)
Build/src/fileUpload.js (1)
Worker/src/index.js (4)
allowedTypes(137-143)file(107-107)formData(106-106)room(109-109)
Build/src/privateMessages.js (2)
Worker/src/index.js (1)
conversationId(34-34)Build/src/main.js (1)
messages(678-678)
Build/src/messageRenderer.js (1)
Build/src/main.js (1)
messages(678-678)
Build/src/search.js (1)
Build/src/main.js (5)
messages(678-678)a(694-694)exportData(684-688)blob(690-692)url(693-693)
Worker/src/chatRoom.js (1)
Worker/src/index.js (4)
room(109-109)user(108-108)url(7-7)conversationId(34-34)
Build/src/main.js (8)
Build/src/soundManager.js (1)
SoundManager(1-69)Build/src/messageRenderer.js (1)
MessageRenderer(5-256)Build/src/privateMessages.js (1)
PrivateMessageManager(3-305)Build/src/fileUpload.js (1)
FileUploadManager(3-351)Build/src/search.js (1)
SearchManager(1-260)Build/src/notifications.js (1)
NotificationManager(1-356)Build/src/contextMenu.js (1)
ContextMenuManager(1-267)Build/src/moderatorTools.js (1)
ModeratorTools(1-299)
🪛 markdownlint-cli2 (0.17.2)
README.md
20-20: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
21-21: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
22-22: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
23-23: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
24-24: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
25-25: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
26-26: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
42-42: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
43-43: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
43-43: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
44-44: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
44-44: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: htmlchat
🔇 Additional comments (5)
Build/.gitignore (1)
1-24: LGTM.Standard ignores fit the new Vite setup and Pages deploy workflow.
README.md (1)
4-4: Version consistency check — confirm Worker /health versionREADME.md shows Version: v0.2.0; Build/package.json contains "version": "0.2.0" (line 3). No version string was found under Worker/src in the repo search. Ensure the Worker /health version matches README or explicitly document that app and backend use separate versioning.
Worker/wrangler.toml (1)
1-14: Approve — DO SQLite migration and class name match; no conflicting migration tags found.wrangler.toml declares CHAT_ROOM (class_name = "ChatRoom") and [[migrations]] tag = "v1" with new_sqlite_classes = ["ChatRoom"]; ChatRoom is defined at Worker/src/chatRoom.js:21.
Build/index.html (1)
37-56: Confirmed — icons are created programmatically (no CDN/global createIcons dependency).Build/src/main.js contains the data-lucide replacement routine (querySelectorAll('[data-lucide]') and helper for creating icons); Build/src/messageRenderer.js, Build/src/fileUpload.js and Build/src/privateMessages.js import icons from 'lucide'; package.json lists the lucide dependency.
Build/src/notifications.js (1)
178-183: Verify icon asset resolution under ViteHardcoded
'icons/icon-512x512.png'may 404 after bundling. Prefer a build-resolved URL.Example alternatives:
- If file is in public/: use
icon: '/icons/icon-512x512.png'- If co-located:
const appIcon = new URL('../icons/icon-512x512.png', import.meta.url).hrefWould you like me to patch this once you confirm the icon location?
Also applies to: 196-203
I HATE CORS I HATE CORS I HATE CORS-
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
Build/src/search.js (1)
193-204: XSS fix: escape-then-highlight is correctly appliedThe new escapeHtml + regex highlight flow addresses the prior injection. Keep it consistent across all text fields.
Build/src/main.js (1)
391-410: Welcome banner now built with DOM APIs — resolved XSSGood switch away from innerHTML; matches the earlier recommendation.
🧹 Nitpick comments (22)
Build/src/fileUpload.js (2)
268-273: Pass auth headers on upload (when AUTH is enabled).If the Worker is configured with AUTH_SECRET, uploads will lack X-Auth-* headers. Add optional auth headers without setting Content-Type (to not break FormData).
Apply:
- const uploadRes = await fetch(`${this.app.baseURL}/upload`, { - method: 'POST', - body: formData - }); + const uploadRes = await fetch(`${this.app.baseURL}/upload`, { + method: 'POST', + // Ensure getAuthHeaders() does NOT set Content-Type for multipart + headers: this.app.getAuthHeaders ? this.app.getAuthHeaders(false) : undefined, + body: formData + });
128-136: Client/server allowedTypes are identical — verification passed.
Verified Build/src/fileUpload.js and Worker/src/index.js contain the same list (11 MIME types).
Optional: centralize the list or add a CI assertion to prevent future drift.Build/src/messageRenderer.js (3)
165-166: Duplicate “system” class append.You append the system class twice.
Apply:
- if (message.system) messageClass += ' system'; - if (message.system) messageClass += ' system'; + if (message.system) messageClass += ' system';
139-139: Remove debug log.Avoid noisy logs in render path.
- console.log('Reply parsed:', { messageId, replyUser, messageText }); // Debug + // Reply parsed
101-108: Consider forbidding inline styles in user content.Allowing “style” increases XSS surface; DOMPurify mitigates most issues, but removing it tightens posture.
- ALLOWED_ATTR: ['href','src','alt','title','target','style'] + ALLOWED_ATTR: ['href','src','alt','title','target','rel']Build/src/privateMessages.js (1)
280-282: Include auth headers for PM history fetch (if enabled).Mirror sendPM headers to avoid 403 when AUTH is configured.
- const res = await fetch(`${this.app.baseURL}/pm/${encodeURIComponent(conversationId)}?user=${encodeURIComponent(this.app.user)}`); + const res = await fetch(`${this.app.baseURL}/pm/${encodeURIComponent(conversationId)}?user=${encodeURIComponent(this.app.user)}`, { + headers: this.app.getAuthHeaders ? this.app.getAuthHeaders() : undefined + });Worker/src/chatRoom.js (7)
581-611: Kick system message says “banned” (but it’s a kick).Text is misleading to users.
- text: `${targetUser} was kicked by ${moderator}${reason ? ` (${reason})` : ''} - banned for 5 minutes`, + text: `${targetUser} was kicked by ${moderator}${reason ? ` (${reason})` : ''} - kicked for 5 minutes`,Also applies to: 616-620
331-362: Handle kicked user on presence heartbeat.updateUserPresence can return {error}; PUT path ignores it and still returns success.
- await this.updateUserPresence(room, user); + const pres = await this.updateUserPresence(room, user); + if (pres && pres.error) { + return textResponse(pres.error, 403); + }
595-600: Case-insensitive presence removal on kick.Presence keys aren’t normalized; deleting with exact case may fail.
- delete users[targetUser]; + const k = Object.keys(users).find(u => u.toLowerCase() === targetUser.toLowerCase()); + if (k) delete users[k];
31-47: Moderator storage is initialized but unused.You seed/maintain moderators in storage, but isModerator() and GET /mod hardcode NellowTCS. Either use storage or remove the dead paths.
- async isModerator(username) { - // Only NellowTCS is allowed moderator access - return username && username.toLowerCase() === 'nellowtcs'; - } + async isModerator(username) { + const moderators = (await this.state.storage.get('moderators')) || []; + return !!(username && moderators.includes(username.toLowerCase())); + } @@ - const moderators = ['NellowTCS']; // Only NellowTCS is moderator + const moderators = (await this.state.storage.get('moderators')) || []; @@ - return textResponse('Cannot modify moderators - NellowTCS is the only moderator', 403); + return this.addModerator(targetUser, verifiedUser); @@ - return textResponse('Cannot modify moderators - NellowTCS is the only moderator', 403); + return this.removeModerator(targetUser, verifiedUser);Also applies to: 166-169, 536-545, 623-646
232-241: CORS headers: include the same allow-headers as JSON/text helpers.Align OPTIONS with jsonResponse/textResponse (missing methods/headers parity).
- 'Access-Control-Allow-Methods': 'GET, POST, PUT, DELETE, OPTIONS', - 'Access-Control-Allow-Headers': 'Content-Type, X-Auth-Token, X-Auth-User' + 'Access-Control-Allow-Methods': 'GET, POST, PUT, DELETE, OPTIONS', + 'Access-Control-Allow-Headers': 'Content-Type, X-Auth-Token, X-Auth-User'(If you later adopt Authorization, add it here too.)
29-33: Durable Object timers: prefer alarms over setInterval.setInterval keeps the DO hot; use DO alarms for periodic cleanup to reduce cost and improve reliability.
312-316: Use slice instead of deprecated substr.Small cleanup.
- id: `msg_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`, + id: `msg_${Date.now()}_${Math.random().toString(36).slice(2, 11)}`, @@ - id: `pm_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`, + id: `pm_${Date.now()}_${Math.random().toString(36).slice(2, 11)}`,Also applies to: 493-499
Build/src/contextMenu.js (1)
19-24: Robust menu click handling.Use closest('[data-action]') so clicks on child elements work.
- this.menu.addEventListener('click', (e) => { - const action = e.target.dataset.action; + this.menu.addEventListener('click', (e) => { + const el = e.target.closest('[data-action]'); + const action = el && el.dataset.action; if (action && this.currentMessage) { this.handleAction(action); } });Build/src/moderatorTools.js (1)
171-182: Banned-words placeholder.Current list is trivial and may overblock “spam” in normal text. Consider disabling or making it configurable per room.
Build/src/search.js (4)
135-141: Robust sort on timestamp strings or numbersHandle both numeric epoch and ISO strings to avoid NaN and ensure correct ordering.
Apply this diff:
- filteredMessages = filteredMessages.sort((a, b) => b.time - a.time); + filteredMessages = filteredMessages.sort((a, b) => { + const ta = Number(a.time) || Date.parse(a.time) || 0; + const tb = Number(b.time) || Date.parse(b.time) || 0; + return tb - ta; + });
162-162: Prefer canonical message id if presentUse msg.id or msg.messageId when available to improve jump reliability across renderers.
Apply this diff:
- const messageId = `msg-${msg.time}-${index}`; + const messageId = String( + msg.id ?? msg.messageId ?? `msg-${Number(msg.time) || Date.parse(msg.time) || 0}-${index}` + );
237-241: Timestamp equality check may fail due to type mismatchCoerce both sides to number for consistent matching.
Apply this diff:
- const messageIndex = messages.findIndex(msg => msg.time === timestamp); + const messageIndex = messages.findIndex(msg => { + const t = Number(msg.time) || Date.parse(msg.time) || 0; + return t === Number(timestamp); + });
154-173: Rendering via innerHTML is still an XSS sink; build DOM nodes insteadYou’re escaping text before highlighting (good), but constructing the entire result with innerHTML keeps risk surface area high (style attributes, future edits). Prefer programmatic DOM creation with textContent and setAttribute.
Example rewrite (illustrative):
- const html = messages.map((msg, index) => { ... return `...${highlightedUser}...${highlightedText}...`; }).join(''); - this.searchResults.innerHTML = headerHtml + html; - this.searchResults.querySelectorAll('.search-result-item').forEach(item => { ... }); + this.searchResults.textContent = ''; + const header = document.createElement('div'); + header.style.cssText = 'padding:8px;background:#f0f0f0;border-bottom:1px solid #ddd;font-weight:bold;'; + header.textContent = `Found ${messages.length} message${messages.length === 1 ? '' : 's'}`; + this.searchResults.appendChild(header); + const frag = document.createDocumentFragment(); + messages.forEach((msg, index) => { + const date = new Date(msg.time).toLocaleString(); + const color = this.app.messageRenderer.getUserColor(String(msg.user ?? '')); + const messageId = String(msg.id ?? msg.messageId ?? `msg-${Number(msg.time)||Date.parse(msg.time)||0}-${index}`); + const item = document.createElement('div'); + item.className = 'search-result-item'; + item.dataset.messageId = messageId; + item.dataset.timestamp = String(Number(msg.time) || Date.parse(msg.time) || 0); + const top = document.createElement('div'); + top.style.cssText = 'display:flex;justify-content:space-between;align-items:center;margin-bottom:4px;'; + const userSpan = document.createElement('span'); + userSpan.style.color = color; + userSpan.style.fontWeight = 'bold'; + userSpan.innerHTML = this.highlightSearchTerms(String(msg.user ?? ''), query); + const timeSpan = document.createElement('span'); + timeSpan.style.cssText = 'color:#666;font-size:12px;'; + timeSpan.textContent = date; + top.append(userSpan, timeSpan); + const textDiv = document.createElement('div'); + textDiv.innerHTML = this.highlightSearchTerms(String(msg.text ?? ''), query); + item.append(top, textDiv); + item.addEventListener('click', () => this.jumpToMessage(messageId, Number(item.dataset.timestamp))); + frag.appendChild(item); + }); + this.searchResults.appendChild(frag);Build/src/main.js (3)
294-303: Duplicate activity listeners registered twicesetupEventListeners and setupActivityTracking both attach the same handlers; remove one to avoid double updates.
Apply this diff to keep the dedicated method only:
- // Activity tracking - ["click", "keypress", "scroll", "mousemove"].forEach((event) => { - document.addEventListener( - event, - () => { - this.lastActivity = Date.now(); - }, - { passive: true } - ); - });Also applies to: 325-336
487-500: Production logs leak auth presence; downgrade to debug or guard behind a flagReduce noisy logs and avoid hinting at auth paths.
Apply this diff:
- console.log('Adding auth headers:', { user: this.user, hasToken: !!this.authToken, includeContentType }); + console.debug('Adding auth headers'); @@ - console.log('No auth headers added:', { - user: this.user, - isNellowTCS: this.user && this.user.toLowerCase() === 'nellowtcs', - hasToken: !!this.authToken - }); + console.debug('No auth headers added');
721-725: Nit: avoid innerHTML for the static “Loading messages…” stubSafe as-is, but keep consistency with DOM APIs.
Apply this diff:
- this.elements.chatBox.innerHTML = - '<div class="msg system"><span class="time">[--:--]</span><span class="user">*** System ***</span><span class="text">Loading messages...</span></div>'; + const sys = document.createElement('div'); + sys.className = 'msg system'; + sys.innerHTML = '<span class="time">[--:--]</span><span class="user">*** System ***</span><span class="text">Loading messages...</span>'; + this.elements.chatBox.textContent = ''; + this.elements.chatBox.appendChild(sys);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Build/src/contextMenu.js(1 hunks)Build/src/fileUpload.js(1 hunks)Build/src/main.js(1 hunks)Build/src/messageRenderer.js(1 hunks)Build/src/moderatorTools.js(1 hunks)Build/src/notifications.js(1 hunks)Build/src/privateMessages.js(1 hunks)Build/src/search.js(1 hunks)Build/vite.config.js(1 hunks)Worker/src/chatRoom.js(7 hunks)Worker/src/index.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Build/vite.config.js
- Worker/src/index.js
- Build/src/notifications.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: NellowTCS
PR: HTMLToolkit/HTMLChat#2
File: Worker/src/chatRoom.js:244-248
Timestamp: 2025-09-18T07:10:50.415Z
Learning: NellowTCS prefers to keep the HTMLChat chatroom open and publicly accessible without authentication requirements, declining authentication/authorization suggestions in favor of an open chatroom architecture.
🧬 Code graph analysis (7)
Build/src/privateMessages.js (1)
Worker/src/index.js (1)
conversationId(34-34)
Build/src/messageRenderer.js (1)
Build/src/main.js (1)
messages(874-874)
Build/src/contextMenu.js (1)
Worker/src/index.js (1)
room(125-125)
Build/src/fileUpload.js (1)
Worker/src/index.js (4)
allowedTypes(153-159)file(123-123)formData(122-122)room(125-125)
Build/src/search.js (1)
Build/src/main.js (5)
messages(874-874)a(890-890)exportData(880-884)blob(886-888)url(889-889)
Worker/src/chatRoom.js (1)
Worker/src/index.js (4)
room(125-125)user(124-124)url(7-7)conversationId(34-34)
Build/src/main.js (8)
Build/src/soundManager.js (1)
SoundManager(1-69)Build/src/messageRenderer.js (1)
MessageRenderer(5-275)Build/src/privateMessages.js (1)
PrivateMessageManager(3-340)Build/src/fileUpload.js (1)
FileUploadManager(3-381)Build/src/search.js (1)
SearchManager(1-279)Build/src/notifications.js (1)
NotificationManager(1-363)Build/src/contextMenu.js (1)
ContextMenuManager(1-265)Build/src/moderatorTools.js (1)
ModeratorTools(1-306)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: htmlchat
🔇 Additional comments (2)
Build/src/fileUpload.js (1)
167-181: Good: filename escaping + stable upload button.Preview now escapes file names and uses a dedicated Upload button id. This closes the XSS hole surfaced earlier and prevents targeting the wrong button.
Also applies to: 184-197, 207-214
Build/src/main.js (1)
506-519: Sanitizer verified — renderMessages uses DOMPurify before assigning innerHTMLBuild/src/messageRenderer.js calls marked.parse then DOMPurify.sanitize (with ALLOWED_TAGS configured); innerHTML assignments are in Build/src/main.js (around lines 512–515 and 573–576).
1 participant
HTMLChat v0.2.0
Highlights
Improvements
Fixes
Build & Deploy
Build/project (npm run buildcreatesBuild/dist)Build/disttogh-pagesusingpeaceiris/actions-gh-pages@v4Breaking Changes
createIcons, nodata-lucidereliance)Build/distLocal Dev
Build/:npm cinpm run dev(Vite dev server)npm run buildfor productionNotes
Summary by CodeRabbit
New Features
Backend
Documentation
Chores