Point two tests at the vendored nostr-tools - #296
Merged
Conversation
search-entity.test.js and web-comment.test.js both did
`require('nostr-tools')`. package.json has declared no dependencies since
3b9d482 on 2026-07-13, which removed the devDependency as part of moving to
vendored copies. Both files were written after that — 2026-07-28 and
2026-07-29 — against a package that was already gone.
So they never ran. Not "failed": the require threw before the file loaded, so
node --test reported one failing file and none of the assertions inside ever
executed. That is 51 tests, including the NIP-22 tag-shape checks that were
written precisely because getting them wrong lands a comment outside the
thread it was meant for, silent for six weeks.
Loading nostr-tools.js the way owner-sign.test.js and the keystore tests
already do fixes it without reintroducing a dependency, and tests the exact
bytes the extension ships rather than whatever npm would have resolved.
Suite goes 1032 pass / 2 fail to 1073 pass / 0 fail.
dmnyc
added a commit
that referenced
this pull request
Sep 9, 2026
A theme belongs to an account, not to the install (themeBy, #266/#303), so reading only settings.theme would have handed these pages the default that an account which never chose inherits. That is this branch's own bug back again for anyone using per-account themes: on Par Avion, opening the guide would still give you a dark page out of nowhere. theme-boot now resolves sidecar_active_pubkey against themeBy and falls back to settings.theme. Both keys are a plain chrome.storage.local read, so nothing has to be asked of the background. RESOLVED HERE, unlike the pay card, and the difference is worth stating because they look like the same problem. That card renders into a web page that can see it, so it must never learn which account is active and takes the theme of the account its own site is bound to instead. help.html, welcome.html and wallets.html are chrome-extension:// documents that nothing on the web can read, so the account on screen is simply the right answer. The change is a no-op until #303 lands: with no themeBy in storage the first term is undefined and it falls through to settings.theme, which is exactly what shipped before. So this does not care which of the two merges first, and the two branches share no files, meaning they would otherwise have merged clean and been silently wrong. The storage listener now watches sidecar_active_pubkey as well, and re-reads rather than pulling a value out of the change record, because the answer needs both keys and each one moves alone. Switching account now repaints a guide already open in another tab, the way a theme change already did. Verified in a browser across all four states: no themeBy at all wears the global and keeps .theme-light; giving the active account Nixie repainted an already-open help.html live and dropped the class; switching to an account on Bauhaus followed and restored it; clearing that account's theme fell back to the default. Suite: 1,034 pass, with search-entity and web-comment still red on the pre-existing missing nostr-tools that #296 fixes.
dmnyc
added a commit
to ChadFarrow/sidecar
that referenced
this pull request
Sep 11, 2026
Three things, one surface. The bell sheet was read-only apart from a Reply button, and the rows are summaries: a 140-character snippet, or — for a reaction, a repost or a zap — no content at all, just "reacted to your note" and a pointer you could not follow without leaving for a client. REFRESH, beside the sheet's close button. Notifications arrive on a live subscription, so this looks redundant until you hit what the live sub cannot cover: the worker evicted while the panel sat idle, or a relay that dropped and reconnected. The sheet opens from cache with no round trip, so there was no way to ask. It goes back through the SAME addEvent the subscriptions use, exposed as cache.refetch, because that function drops your own events, applies the mute list, de-dupes by id, prefetches the sender's name and appends to an open sheet. A second copy of those five would be five things to keep in step, and the ones that fail quietly — a muted sender reappearing, a duplicate row — are the ones that survive a release. Nothing new is the usual answer, so it says so rather than going silent. REPLY, REACT AND ZAP on every note row, as three icons. Not behind the chevron: they are what you came to the row to do, and a drawer in front of them is a tap that buys nothing. Which is affordable precisely because they are icons — three labelled buttons is ~200px of minimum width and reads as a toolbar repeated 25 times down a list, where the glyphs this app already uses for these verbs read on their own and carry an aria-label each. They are 30px tap targets rather than shrunken buttons, and offered on a note or a comment only: there is no thread to join on a kind:7, and a button that produces a dead-end event is worse than no button, which is the rule the old Reply already followed. React opens a full picker over the vendored Unicode table, 1,914 emoji in nine groups with search across all of them. One group at a time: every cell is a real button, for the keyboard and for a screen reader, and 1,914 of those is a visible hitch on open. Zap sends a zap OF THE NOTE rather than of its author, by handing makeZapRequest the event so it carries an e tag, and reuses the profile sheet's presets and amount row so the two forms cannot drift apart. Its form is built on first tap, since learning whether someone can be zapped at all costs a profile fetch and doing that per row would be 25 round trips for the one zap anybody sends. EXPAND, on a chevron in the row's corner, is left with the job the actions never belonged to: reading. A reply or a mention gives up its full text; a reaction, repost or zap fetches the note it is about and quotes your own note back at you, which is the half of the sentence the label leaves out. Notes are cached including the misses, since two reactions to one note is the normal shape of a busy list. Worth knowing: the text is cut TWICE, at 140 characters in the source and again by a three-line -webkit-line-clamp in CSS. Undoing only the first left the row looking identical, with the browser drawing its own ellipsis at the same three lines, so the chevron read as a control that did nothing. emoji-data.js joins the vendoring pipeline with the same gates as the rest: pinned version, tarball checked against the registry's integrity hash, hash recorded in scripts/vendor-hashes.sha256, provenance in VENDOR.md. It is reduced rather than copied — the published file is 838KB because it carries a slug, two version fields and a skin-tone flag per emoji, and a picker needs the character, a name to search and the group. Packed, it is 52KB. Verified in a browser end to end, with a real note from a second account: the row arrived carrying its three icons unexpanded, the chevron grew the text from 141 to 205 characters with the clamp lifted, React published a kind:7 🔥, and Zap opened its form on someone with no lightning address to the honest "They have no lightning address." with Send held disabled. 27 new tests across notif-refresh and notif-actions; suite 1,109 pass, with the two pre-existing nostr-tools failures that dmnyc#296 fixes.
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.
Both files did
require('nostr-tools').package.jsonhas declared no dependencies at all since3b9d482on 2026-07-13, which removed the devDependency as part of moving to vendored copies. Both test files were written after that — 2026-07-28 and 2026-07-29 — against a package that was already gone.So they never ran. Not "failed" in the sense of assertions going red: the
requirethrew at load, sonode --testreported one failing file and nothing inside ever executed. That's 51 tests, including the NIP-22 tag-shape checks that exist precisely because getting that wrong lands a comment outside the thread it was meant for. Silent for six weeks, across four releases.The fix loads
nostr-tools.jsthe wayowner-sign.test.jsand the keystore tests already do. No dependency comes back, and it tests the exact bytes the extension ships rather than whatever npm would have resolved — which for a signer is the version you actually want under test.Suite goes from 1032 pass / 2 fail to 1073 pass / 0 fail.
I also swept for other tests requiring a package that isn't installed. These two were the only ones.
One thing I did not change, because it's a judgement call rather than a fix:
package.jsonsays1.10.0whilemanifest.jsonsays1.12.0. Nothing reads the former —scripts/stamp-version.shand the packaging scripts use the manifest — so it's cosmetic drift rather than a bug. Worth either syncing it at release time or dropping the field so it can't mislead.