refactor(chat): Stage 2a — remove cruft, fix desktop composer floating#22
Merged
refactor(chat): Stage 2a — remove cruft, fix desktop composer floating#22
Conversation
Net -23 lines across 5 files. Reverts several defensive patterns that were added to work around what we thought was an iOS PWA reactivity bug but (per last night's three-agent review) was actually a stale service worker shell. Stage 1 (PR #21) shipped the real SW fix; this PR restores the code to a clean form and fixes one real layout bug that was exposed along the way. Removed: - Server-side User-Agent `isMobile` detection in +layout.server.ts. Pushed a presentation concern into server data + broke DevTools mobile emulation. Replaced with matchMedia in ProjectChats, seeded at script-init time (`typeof window !== 'undefined'`) so SSR gets `false`, client picks up the correct value on hydration. `$effect` handles viewport changes. - `inChatThread` pathname regex in projects/[slug]/+layout.svelte. Layout shouldn't know about child routes. Replaced with a CSS `:has([data-chat-thread])` rule in app.css, scoped to `@media (max-width: 767px)` so desktop keeps the project chrome (where the two-panel layout needs it) while mobile hides it for the full-screen thread takeover. - `textareaEl.value = ''` and `textareaEl.value = previousDraft` DOM writes in chats/[threadId]/+page.svelte. Papering over bind:value which works fine now that Stage 1 keeps hydration stable. - `const text = (textareaEl?.value ?? draft).trim()` in sendCurrentMessage. Same — trust the framework. - Explicit `draft = el.value` in handleTextareaInput. bind:value does this itself. - `isMobile` prop resolution via `$page.data.isMobile`. Back to a simple test override only. Fixed (real bug): - Desktop composer was floating mid-screen instead of pinning to the bottom of the messages pane. Root cause: the tab-content div `flex-1 overflow-y-auto` was missing `min-h-0`, so `h-full` on descendants resolved to content height rather than the viewport slot. Added `min-h-0` to the tab-content div and the desktop ProjectChats root. This is pre-existing, was masked by the broken build, and surfaced when PR #15 unstuck deploys. Restored: - `disabled={sending || draft.trim() === ''}` on the send button. Stage 1 restores hydration reliability, so bind:value propagates and the reactive expression re-evaluates correctly. - CSS `:has()` rule for chat-thread takeover now lives wrapped in a mobile-only media query, addressing the auditor's concern that the project chrome was globally hidden (regressing desktop when directly visiting a thread URL). All 80 unit tests pass. `isMobile: true` prop in mobile-chat.test.ts is still honored via the `props.isMobile` precedence path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 53 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…dration) Root cause of tonight's entire symptom cluster (send button dead, back button dead, buttons unresponsive, hydration "broken" on iOS PWA AND desktop). The Traefik CSP `script-src 'self'` header introduced by PR #15 blocked SvelteKit's own inline hydration script: <script> Promise.all([ import("./_app/immutable/entry/start.<hash>.js"), import("./_app/immutable/entry/app.<hash>.js") ]).then(([kit, app]) => { kit.start(app, element, { node_ids:..., data:..., form:null, error:null }); }); </script> That script is necessarily inline because it carries page-specific hydration data (node_ids, loaded data, form state, error). CSP blocks it → JS never hydrates → no event listeners attach. Browsers render the SSR HTML just fine (so <textarea> accepts text, <a href> links navigate) but <button onclick> handlers are wired up by the blocked hydration and do nothing. This is NOT a Svelte 5 / iOS PWA reactivity bug (the memory I kept citing was based on the same misread from a prior session). It manifested on EVERY platform identically, it was just hidden by the broken build from 4/17 to tonight. Hot-patched live via ssh + sed on KVM 2 earlier; this commit brings the repo in line so the next deploy doesn't revert the fix. Follow-up (non-urgent): swap 'unsafe-inline' for SvelteKit's built-in `kit.csp` config (svelte.config.js), which injects sha256 hashes of the specific inline scripts — strictly more secure than unsafe-inline. Tracked separately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Net -23 lines across 5 files. Stage 2a of the chat refactor: remove defensive patterns that were working around a stale-service-worker symptom cluster (Stage 1 / PR #21 was the real fix), and address one latent layout bug that was hidden while the build was broken 4/17→4/23.
Removed cruft
Fixed — real bug
Desktop composer was floating mid-screen, not pinning to bottom of messages pane. Root cause: the project-layout tab-content div `flex-1 overflow-y-auto` was missing `min-h-0`, so `h-full` on descendants resolved to content height rather than the viewport slot. Added `min-h-0` on the tab-content div and the desktop ProjectChats root. Pre-existing, masked by the broken build.
Restored to canonical form
Verification
Mobile-chat.test.ts still works via the `props.isMobile` test override path.
What's NOT in this PR (Stage 2b, future)
Those are the larger component-extraction pass and deserve their own focused PR + review cycle.
🤖 Generated with Claude Code