Repository navigation
feat(widget): add copy action to completed replies - #504
Conversation
anurag629
left a comment
There was a problem hiding this comment.
Thanks @piyusshhjangid, this covers what #477 asks for, and the translations are a nice touch. With dev merged in: build, typecheck, lint (38, same as dev), api:check, docs:check, coverage and the 79 e2e tests in Chrome all pass. pnpm size fails, though:
- panel 17.93 of 18 kB
- single file 20.05 of 20 kB
#502 has merged since and adds 0.22 kB to both, so on dev now it's further over.
Two things:
packages/widget/src/dom/messages.ts,readableText(): the tree walker after theinnerTextline only runs whereinnerTextis missing. That's jsdom, not any browser the widget supports, so it's test-only code shipped to every page, and it's 0.21 kB of the panel (17.93 to 17.72 without it). Make the functionreturn element.innerText.trim();and give the tests aninnerText. For example, in the test file'sbeforeEach:Or stub it on the one element in the formatted-content test, which is closer to what a browser returns.Object.defineProperty(HTMLElement.prototype, 'innerText', { configurable: true, get() { return this.textContent; } });
- The budget itself: even trimmed, this plus #502 puts the single file just over 20 kB. That's on me, not you. I'm opening a PR now to make room in the panel and the single file. Once it's merged, update your branch from dev, and
pnpm sizeshould pass with (1) done.
|
@piyusshhjangid the budgets have room now (#508: panel 19 kB, single file 22 kB). Update your branch from dev after the |
anurag629
left a comment
There was a problem hiding this comment.
Thanks @piyusshhjangid, the innerText change is right, and the copy works end to end. I checked it in Chrome against the e2e server with real clipboard access: Copy puts the reply's text on the clipboard and announces "Reply copied.". With dev merged in:
- build, typecheck,
api:check,docs:check, coverage and all 85 e2e tests pass pnpm sizepasses: panel 18.63 of 19 kB, single file 21.9 of 22 kB
One last thing. Lint is at 39, against 38 on dev, and the new warning is your test:
packages/widget/tests/dom/messages.test.ts:204, lint/style/noNonNullAssertion:
const content = parent.querySelector<HTMLElement>('[data-id="copy-1"] .cc-message-content')!;Drop the ! and check it instead, for example if (!content) throw new Error('no reply content');. Then pnpm lint says 38 and I'll merge.
|
@piyusshhjangid also: dev now has #521, which pushed the single file closer to its limit, so I raised it to 23 kB in #523. Update from dev with the lint fix and |
anurag629
left a comment
There was a problem hiding this comment.
Thanks @piyusshhjangid. Lint is at 38. With dev merged in, everything passes, including all 85 e2e tests. Sizes are fine: panel 18.79 of 19 kB, single file 22.05 of 23 kB.
|
Merged, thanks @piyusshhjangid. With #477 done you have a free slot, so #525 is yours once #524 is in. |
Closes #477
Summary
@chatcops/widget.How I tested this
rm -fcleanup step is unsupported in Windows Command Prompt.pnpm size— passed; panel is 17.75 kB gzipped against the 18 kB limit.git diff --check— passed.AI assistance
OpenAI Codex assisted with implementation and verification. I reviewed the changes and test results.