Repository navigation
perf(widget): embed from one non-blocking self-contained script tag - #79
Fidasek009 wants to merge 9 commits into
Conversation
7e8a9d9 to
7d55961
Compare
faf9e35 to
5346e64
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything — ready to find some rogue Every commit in this round either tightens the code or removes dead weight. The The two test removals (widget-config 404 and retired data-talqo-widget) are defensible: they tested history, not behavior, and the canonical endpoints already carry their own coverage. The 📊 Overall: Like a chef who keeps finding burnt pancakes in the trash and keeps cooking until they're golden. Every commit in this round tightens the code without introducing new problems. Files Reviewed (22 files)
Previous Review Summaries (13 snapshots, latest commit 5c58cf7)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 5c58cf7)Verdict: No Issues Found | Recommendation: Merge Oh wait, this incremental diff is actually clean. I had my flamethrower warmed up and everything — ready to roast some unlayered The selector-validation test ( The two test removals (widget-config 404 and retired data-talqo-widget) are clean: they tested history, not behavior, and the canonical endpoints already carry their own tests. 📊 Overall: Like a chef who keeps finding burnt pancakes in the trash and keeps cooking until they're golden. Every commit in this round tightens the code without introducing new problems. Files Reviewed (4 files in incremental diff)
Previous review (commit ecbbb05)Verdict: No Issues Found | Recommendation: Merge Oh wait, this incremental diff is actually clean. I had my flamethrower warmed up and everything — ready to roast some unlayered The one thing worth noting: the comment on 📊 Overall: Like finding a PR that doesn't need fixing — rare enough that I'm suspicious, but the logic checks out. Files Reviewed (2 files)
Previous review (commit 5802abb)Incremental Code Review: No Issues FoundReview scope: Changes from SummaryThe PR moves from a two-file widget distribution ( Files reviewed (22 changed)
What was checked
Verdict: No issues found. The refactoring is clean, well-tested across all surfaces (unit, integration, E2E, bundle), and correctly removes the old compatibility paths. All entry points (widget, dashboard preview, SDK) remain consistent with the single self-contained script model. Previous review (commit b58f739)Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. 📊 Overall: Like a chef who burned the first pancake, threw it away, burned the second one less, then served the third one perfectly golden. Five commits of progressively cleaning up everything this review process flagged — plugin ordering verified with mutation tests, comments shortened, tests merged into tighter assertions, docs trimmed of vestigial sentences. The ADR history rewrite was politely declined (and honestly, the reasoning holds water), and the plugin order concern was thoroughly debunked with actual artifact verification. This PR went from "fix the critical bug" to "actually nothing left to fix." Files Reviewed (23 files)
Previous review (commit 6ac85c9)Verdict: No Issues Found | Recommendation: Merge Oh wait, this incremental diff is actually clean. I need to sit down. I had my flamethrower warmed up and everything. 📊 Overall: The "agent-level tokens" sentence was vestigial — every token resolves through an embed now, so describing a data model that doesn't exist is just free confusion. Dropping it is the right call. The test merge is also sharper: checking Files Reviewed (2 files in incremental diff)
Previous review (commit 3f81244)Verdict: No Issues Found | Recommendation: Merge Oh wait, this incremental diff is actually clean. I need to sit down. I had my flamethrower warmed up and everything. 📊 Overall: Two files, zero new bugs. The docs drop an "never released" sentence a deployer doesn't care about, and the CSS tests merge into one that checks the injected stylesheet instead of the raw bundle — more precise, less noise. The Files Reviewed (2 files in incremental diff)
Previous review (commit 200f2f7)Verdict: No Issues Found | Recommendation: Merge Oh wait, an incremental diff of only comment removals and shortenings. I had my roast prepared for actual code — and you gave me a whitespace-and-documentation PR. This is like showing up to a food fight with a salad. 📊 Overall: Eight files, zero functional changes. Every removed comment either repeated what the test name already said or explained something the code itself documents. The remaining comments are shorter and sharper. No bugs introduced, no logic changed, no technical debt added. Files Reviewed (8 files in incremental diff)
Previous review (commit fa80ce0)Verdict: No Issues Found | Recommendation: Merge Oh wait, this incremental diff is actually clean. I need to sit down. I had my flamethrower warmed up and everything. 📊 Overall: Like finding a unicorn in production — I didn't think PRs fixed all their issues in a single follow-up, but here we are. The ADR-0017 reference in Files Reviewed (2 files in incremental diff)
Previous review (commit 27b80f6)Verdict: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
Outside Diff
💀 Worst part: The ADR history rewrite. You had a valid ADR-0017 that the previous review approved. Deleting it and amending ADR-0013 in-place erases the paper trail instead of correcting it. 📊 Overall: Like a student who got an A on a paper, then emailed the professor asking to delete the grade from the record because "the citation was never actually published." The reasoning is sound, the method is not. Files Reviewed (5 incremental files)
Previous review (commit 20aa1b5)Verdict: No Issues Found | Recommendation: Merge Oh wait, this incremental diff is actually clean. I had my flamethrower warmed up and everything. 📊 Overall: Clean removal of the legacy compatibility adapters. The ADR files are well-documented, the test coverage is appropriate (the widget test for the retired attribute is a nice defensive guard), and the API surface shrinks cleanly. No dangling references, no forgotten CORS middleware, no auth holes. Files Reviewed (11 files in incremental diff)
Previous review (commit d9bb82f)Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. 📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are. The "ships no unscoped css" test at Files Reviewed (16 files)
Previous review (commit 5d027b7)Verdict: No Issues Found | Recommendation: Merge Oh wait, this incremental diff is actually clean. I had my flamethrower warmed up and everything. 📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are. The "ships no unscoped css" test at Files Reviewed (1 file in incremental diff)
Previous review (commit 5346e64)Verdict: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The mount-deferral test ( 💀 Worst part: The plugin ordering bug undoes the entire point of the PR. If this ships, every customer page gets un-scoped Tailwind global styles leaking through the widget. The CSS injection works, but the scoping doesn't — so you traded two requests for one, but the one request now trashes host pages. 📊 Overall: Like cooking a gourmet meal and then serving it on a dirty plate. The substance is right, but a single line of Files Reviewed (14 files)
Reviewed by deepseek-v4-flash · Input: 141.5K · Output: 12.5K · Cached: 970K |
20aa1b5 to
27b80f6
Compare
b58f739 to
5802abb
Compare
The generated snippet was parser-blocking, so any page carrying a Talqo widget stalled its own render on a third-party bundle. The loader already tolerates late execution: document.currentScript stays populated while an async classic script runs, and it defers mounting until the document is parsed. The build now folds the stylesheet into the bundle, so one tag is the whole embed: one request instead of two, and no window where the widget could paint before its styles arrived. That drops ensureWidgetStylesheet along with the URL rewriting that had to version-match the stylesheet by hand. It adds no CSP requirement: the widget already applies its palette through an inline style prop. Net 111,810 bytes gzip in a single request, against 112,810 across two. The static responder drops its /widget.css route, which no longer exists. Co-Authored-By: opencode (noreply@opencode.ai)
Store each public token and access version on an embed and derive its agent server-side, so the public surface is a single script attribute and a single configuration endpoint. The SDK already calls only `/api/embed-config/:token`; the extra attribute and route carried their own CORS grant and authentication exemption for no caller. ADR-0013 states the decision. Co-Authored-By: opencode (noreply@opencode.ai)
Drop the ones a test name already carries, and cut the rest to one line. The four that stay each record something not derivable from the code: plugin order decides whether the shipped CSS is scoped, Tailwind expands CSS after any transform hook, and honouring the retired embed attribute would fire a request. Co-Authored-By: opencode (noreply@opencode.ai)
A deployer copies a generated snippet and does not care that an unreleased attribute once existed, so that sentence goes. What is left is what they actually decide: keep async, and when to omit data-talqo-api. The two CSS tests collapse into one that reads the injected stylesheet rather than the bundle text, which is both more precise and shorter, and the assertion that no widget.css file is emitted goes with it: nothing loads that file, so its absence is a layout detail rather than behaviour. Co-Authored-By: opencode (noreply@opencode.ai)
Every embed token resolves through the embed, and an embed selects one agent, so there is no agent-level token for a reader to wonder about. The sentence described a distinction the data model no longer makes. Co-Authored-By: opencode (noreply@opencode.ai)
Access is declared per route with access.public, so the old "authentication exemption" wording no longer describes the mechanism. Co-Authored-By: opencode (noreply@opencode.ai)
Cascade layers resolve before specificity, so leaving our utilities in Tailwind's @layer utilities let a host's @layer base preflight win: background, border, font-size and padding all reset on a Tailwind page while the same widget was correct on any other host. Unlayering every non-base block is the whole fix. Verified against a real Tailwind v4 stylesheet: the widget now renders identically with and without one on the page. Co-Authored-By: opencode (noreply@opencode.ai)
The 404 for a removed route and the network silence for a retired attribute described history rather than behaviour. The canonical embed-config endpoint and the no-token mount already carry their own tests, so both removals lose nothing. The CSS check now asserts the shipped stylesheet directly: it contains scoped rules, it participates in no cascade layer, and every selector it carries is scoped to the widget. The leak strings it replaced named specific past outputs. Co-Authored-By: opencode (noreply@opencode.ai)
1b16692 to
f5e637c
Compare
1 participant
Summary
Three changes.
The embed snippet was a parser-blocking
<script src>, so any page carrying a Talqo widget stalled its own render on a third-party bundle. It is now<script async>, and the build folds the stylesheet into the bundle, so one tag is the entire embed: one request instead of two, and no window where the widget paints before its styles arrive. Net 112,010 bytes gzip in a single artifact, against 112,810 spread acrosswidget.jsandwidget.css; the static responder drops its now-dead/widget.cssroute.Each public token and access version now lives on an embed with its agent derived server-side, leaving one script attribute and one configuration endpoint (
ADR-0013). The SDK already called only/api/embed-config/:token; the extra attribute and route carried their own CORS grant and authentication exemption for no caller.The shipped CSS is no longer wrapped in cascade layers. A Tailwind host's
@layer basepreflight was outranking the widget's layered utilities and clearing background, border, font size and padding off the chat input, while the same widget was correct everywhere else. Unlayering our output fixes it with no change to the no-host case.The load-bearing assumptions each have a test that fails when the thing it guards is removed: the loader defers its mount until the document is parsed, and the shipped stylesheet carries scoped rules in no cascade layer.
How to test
.talqo-widget.Content-Security-Policy: style-src 'self', which rejects inline styles. Expect the same result: the widget already sets its palette through an inline style attribute, so it required'unsafe-inline'before this change too.Checklist