Repository navigation
Open-source readiness: review fixes, README, licensing and docs - #12
Merged
Merged
Conversation
WolframAlpha's example read `wa 42 miles in km`, but its only keyword is `wolfram` and `wa` belongs to WhatsApp. The browse list prints the example verbatim under each row, so the page was telling people to type a keyword that opens somebody else's site. WhatsApp had the mirror of the same slip, showing `whatsapp` when its keyword is `wa`. A test now collects every example whose first word is not one of that command's own keywords, so the next one fails the build. It is one test rather than one per command: a failure should name every row that drifted, not just the first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It opened with install instructions and buried what the thing is. A stranger now gets two sentences and an example first, then the not-affiliated-with-Meta line, then install, then the one design decision that explains the rest: the first word of a query is always a command when it matches a keyword, and the escape hatch that makes that liveable. Corrections found while checking the text against the code: the GitHub username setting is read only by `gh me`, not by `pr` and `iss`; the fallback engine has five presets, not two; the shipped table was missing Goodreads and the meta keywords; the first run has a Skip button the text did not mention. Adds CI and licence badges, the pinned Node and pnpm versions, and a marked placeholder for the three screenshots the repository still needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A security pass found `gh facebook/react constructor` building `https://github.com/facebook/react/function Object() { [native code] }`. `GITHUB_TABS` was a plain object literal, so a lowercase Object.prototype key answered truthy and was interpolated into the path. Nothing escaped the origin and nothing was exploitable: the value always lands after the encoded repo path and carries no ? or #, so the worst case was a 404 from a query nobody types. But this is exactly the shape AGENTS.md invariant 17 exists for, in a module the invariant does not name, so all five tables that an argument or a user-edited URL can index are null-prototype now: the two GitHub ones, the AI aliases, the Drive app types and the meta route parameters. `normalizeTemplates` in storage.ts joins them. It was the one override map the parser built on a plain object, and a string assigned to `__proto__` there is swallowed by the inherited setter rather than stored, so a key could go missing without the parser saying so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It enumerated two stores and the extension uses three. The options page also writes `bunnylol.collapsed` to its own `localStorage`, holding the list of folded shortcut groups. The substance of the policy was never in doubt, since that value is per-machine view state that never leaves the machine, but a document whose whole worth is that it is complete cannot omit a store a reader will find by grepping for the API. Also corrects the tab attribution. Opening a tab was credited to the popup alone; the omnibox and the install-time welcome tab do it too. All three use only create and update, so the load-bearing claim, that the `tabs` permission is not requested and no page can be read, is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
docs/handoff.md was written for the next agent working on the release, not for anybody who will read this repository. It cited eleven paths under a gitignored directory that no clone will have, described a branch and a pull request as in flight, listed manual browser checks as still owed, and said of itself that it was probably deleted before the merge. It was not. Publishing it advertises unfinished QA and explains nothing a contributor needs. The one durable section was its list of non-obvious facts that bit somebody during development. The seven that were not already recorded move into the AGENTS.md rules, which is where a future reader will look: the relative meta URLs, the preview substituting at the registry index, re-minted ids having to be rewritten in disabled and deleted, the vitest css flag that stops the token assertions passing vacuously, the flat-hex accent the icon generator parses, the harness class that must not reach the shipped sheet, and what hasOnboarded means on a profile that never answered the picker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 1.1.0 entry described the status pill twice and incompatibly: once under Changed as saying "Shortcuts active", once under Removed as having lost that state. The build produces neither string on a healthy profile, because the pill is silent. The two entries are now one that says what the release actually did. The Goodreads line claimed the same release both dropped and restored it, which is true of the development history and useless to a reader. Every link at the foot of the file pointed at a tag that does not exist, so all three 404 on the page a stranger scrolls to. The 1.0.0 link is gone with a note that the version was never published, and the 1.1.0 link now points at a release tag rather than a comparison. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AGENTS.md requires the README and any listing copy to say the project is not affiliated with Meta. Both did. The extension itself did not, and the welcome screen opened with "A rebuild of an internal tool at Meta", which is the strongest affiliation claim anywhere in the project and the only one a user or a store reviewer meets at runtime. The sentence now says an independent rebuild of the command bar used inside Meta, and a quieter line under it disclaims affiliation, endorsement and sponsorship outright. The rule in AGENTS.md is widened to cover the extension's own UI, since that is the place it was just broken. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six things a stranger or a store reviewer would hit. The bug report form required the rule-status text and gave an example string this build cannot produce. On the commonest bug, a healthy profile sending a shortcut to the wrong place, there is no pill at all, so the field could not be answered honestly. It is optional now and says that nothing shown is a fine answer. The shipped zip carried the font licence but not this project's own. LICENSE moves into public/ so the packer picks it up, which is what MIT asks for when the software is redistributed. store/listing.md holds the Web Store copy. The two paragraphs that are compliance text rather than marketing, the search-behaviour disclosure and the non-affiliation line, are quoted from the submission crib verbatim so neither can be softened while somebody is pasting fields into a dashboard under time pressure. The crib itself named the rule-status pill in its suggested screenshot set, which is now a shot of nothing, and told the reader to find a published URL for the privacy policy when the dashboard accepts the file's own GitHub URL. CONTRIBUTING gains what a contributor cannot infer: one maintainer, a week for a reply, what a major and a minor mean here given that the stored state format is the compatibility surface, and the release steps including building the zip fresh rather than trusting one left in release/. Dependabot watches the GitHub Actions pins only. Those are third-party code running against this repository and nobody notices when one goes stale. npm is left out on purpose: four devDependencies that move rarely are the policy, not an oversight. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A review of the Hidden shortcuts work found these, none of which any test could see: the repo has no DOM environment, so every view in it is untested by construction. Start over left the Hidden shortcuts group unfolded. The collapse store records ids whose fold differs from the default, and `expandAll` adds every default-folded id on purpose, because the Expand all button has to be able to open that group. `forgetCollapsed` is not that button, it is the reset that puts a profile back to how it was installed, so a user who declined two packs landed on the page of dead rows the folded default exists to prevent. `CollapseState` gains `reset`. Flipping any switch dropped keyboard focus to the body. `move` restored focus before `applyFilter` had decided visibility, so on the default path, with the hidden group folded, the row was inside a `display: none` subtree where focus is a silent no-op. `move` now returns the element and the caller focuses it after `applyFilter`, which is the order `turnOn` already used. Deleting a row had the same hole with nothing to catch it; focus goes to the filter box. The "omnibox only" badge went stale. It was computed once per render for rows that rendered enabled, so switching a row on never added one and a row kept its badge under Hidden shortcuts. `applyFilter` writes it now, alongside the counts it already owns, off a memoized keyword set. Also corrects two comments that claimed `turnOn` empties the list it walks. It does not: `move` reassigns the array rather than mutating it. A confidently wrong comment is what makes a reviewer skip the bug under it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AGENTS.md invariant 6 says every alias check goes through validate.ts and that a new surface adds a call site rather than a local rule. The Exempt keywords field re-derived one of the three rules by hand, testing only for whitespace. So `\gh` and `=npm` were accepted and stored, where they can never match anything, because resolve strips that prefix before the key map is consulted. The user got a permanent chip that did nothing, and an arbitrarily long paste was stored too. Also from the same review: `commitSettings` swallowed its failures while its two siblings reject, and it did not roll back the state it had already applied optimistically, so a failed write left the page and every render after it showing a value that was not in storage. It now rejects like the others and restores the previous settings slice, not a whole snapshot, so a write that landed while this one was in flight is not undone by its failure. Dead code from the cards and the pill that went away: the `ok` status tone, which nothing can produce now that a healthy sync is silent, and `editedFields`, whose only consumer was its own test. `countShortcuts` moves from a view into lib/text.ts beside `countShipped`, where the helpers every surface shares live. `dispatchToast` keeps its name, since renaming the stored field would read as off for every profile written so far, and gains a comment saying so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
917 lines and 42 functions with no dividers, holding two complete parallel families plus the chrome I/O. They differ in the one way that matters, which is what they do with bad input, and nothing in the file said which one a reader was in. `storage/normalize.ts` is the lenient reader: any blob in, a usable state out, and it never throws. `storage/parse-import.ts` is the strict parser: it refuses a bad file with a message naming what is wrong. `storage/shared.ts` holds what both need. `storage.ts` keeps the I/O, the export and the public surface, so every importer is unchanged and the test file needed no edit at all, not even an import path. Each module docstring now opens with its own bad-input behaviour, since invariant 17 turns on the difference between them. Function bodies and their comments travelled verbatim. Verified by diffing the original against the four files: the only changed lines are the fourteen declarations that gained an export keyword. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
818 lines holding rule construction, the serialized sync state machine, and the RE2 fitting and sharding, with `buildRules` at the top and `fitPlan` five hundred lines below it. Nothing showed that those two are entry points into one set of constructors rather than two copies of them. `dnr/rules.ts` is now the only module that mints a rule, and it has exactly two consumers, both visible from its import graph: `buildRules` beside it, which only tests call, and `fitPlan` in `dnr/fit.ts`, which `syncRules` calls. `fit.ts` defines no rule of its own, so what a `buildRules` test omits is precisely the fitting step, which the docstrings now say outright. `dnr/keywords.ts` holds the ranking, the alternation order and the sharding. Every invariant comment travelled with its own code: the priority tiers, the RE2 pattern that has to swallow the whole URL remainder, the two orders one list rule, the fail-closed precondition, and the trailing slot the serialized rebuild consults before the in-flight one. Verified line by line against the original; the only additions are export keywords. The public surface is unchanged and no test needed an edit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
842 lines, of which `renderBrowse` was 531 holding eight mutually referencing closures over several mutable locals. The rule that `applyFilter` is the only writer of visibility, of every count and of the badge could only be verified by reading all of it, and two shipped bugs lived there for exactly that reason. `browse-row.ts` takes the row, which closed over nothing. `browse-groups.ts` takes the group headings, the runs and the refiling of a row between them. Neither writes anything that is on screen: the headings and the bulk buttons are built empty, and every function that changes what a group holds takes the repaint as a callback. So the rule is now a grep over two short modules. `browse.ts` is 500 lines and no longer imports the storage writers at all. `browse-groups.ts` is plain functions over their arguments rather than the factory the review suggested. A factory closed over the collapse state and a repaint callback would have to be constructed before `applyFilter` exists and then be read by it, which puts a mutable slot on the one seam this contract lives on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Around 3,400 lines of view code were untested by construction: vitest runs under `environment: node` and there was no alternative, so two of the three bugs the last review found were in code no test could reach. jsdom joins the dev tooling, and exactly one suite opts into it with a `// @vitest-environment jsdom` docblock. The global default stays `node`, which is what keeps the rule that lib and model import cleanly without a DOM able to fail. The suite covers the Hidden shortcuts state machine, the newest code in the repo: a switch moves the row between groups and repaints both headings, a section survives losing its last live row, the group leaves the page when its last row is switched on, a bulk action is one write for the whole run, delete drops the row from every total, and the filter force-expands the folded group to reveal a hidden row. Two assertions were mutation-checked: moving the commit inside the bulk loop fails the one-write test, and a bare collapse read fails the force-expand test. The docstring is honest about the ceiling. jsdom does no layout, so `focus()` inside a hidden subtree succeeds there and fails in Chrome, which is precisely the bug this file cannot catch, and the CSS order reordering is invisible to it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The suite was 1401 cases from 776 written blocks. These four were duplicates, and each was checked against the code before it went. Three form-builder cases: the form's `buildCommand` is a four-line wrapper whose only addition is narrowing the category, and the three cases removed were asserting the wrapped builder's own behaviour, which its own test file already covers more strictly. Thirty-five self-interception cases: every sweep in that file ran twice, once over the test-only `buildRules` and once over the rules the sync path registers. Both sets were built over the shipped registry and compared rule by rule: 36 rules each, identical in priority, action and condition, differing only in id. The mirror was catching nothing. Invariant 1 is untouched, and is now derived only from the rules that actually ship, which is the stronger of the two. One manifest-floor case asserted from the token tests what the manifest test asserts more strictly. Its reasoning moved into the surviving comment rather than being lost. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CONTRIBUTING stated the style rules by hand and nothing checked them, so the first outside pull request was going to be a style negotiation in review comments. Prettier is configured from what the repo already does rather than from its defaults. Every setting was measured: print width 100 is the value that touches the fewest files and the fewest lines out of eleven candidates, and trailing commas and arrow parens were chosen the same way. Both stylesheets came through the first run unchanged, which is a fair signal the config matches the house style. design/ is excluded because it is review-gated and its hand-aligned contrast-ratio comments do not survive a formatter, go.html because its inline style block is deliberately minified on the one page whose job is to redirect before it paints, and Markdown because a formatter has nothing to offer prose that is already hand-wrapped. ESLint is flat config and type-aware, which costs about two seconds, so it runs first in CI as the fastest signal. It enforces the two rules a typechecker cannot see and CONTRIBUTING already asked for: no default exports, and `import type` for type-only imports. Every rule turned off is a convention rather than a dodge, and each carries its reason in the config. The largest is the unsafe-assignment rule, which fires on exactly the null-prototype construction that exists to satisfy invariant 17. The one real suggestion it made is fixed here: the import parser now attaches the underlying error as `cause` when it rethrows a JSON parse failure, so the stack survives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Several passes of feature removal and three file splits landed in the last day, each cleaning up after itself and each missing something. This is the sweep, plus the repo-wide Prettier pass, which touched about four and a half percent of the lines and changed no behaviour. Fifteen exports dropped where nothing outside the file called them, so `noUnusedLocals` keeps them honest from now on. No code deleted and nothing renamed. Deliberate test-only seams were left alone, and so was every exported type that names a public signature. The comments were the important half. A confidently wrong comment is worse than none, and this codebase comments heavily. Five referred to "the monolith", a file layout that has not existed for a long time and that a stranger has no way to look up; each now states the constraint directly. Two pointed at a per-shortcut Restore that was removed. Two named `weather`, `gimg` and `gsite` as the commands at risk of self-interception, and none of those three still exists, so both were rederived by running the current registry through the current rules. Four pointed at modules the splits moved code out of. Two quoted keyword and shard counts that were off by a factor. Four entries in the token tests still described the dispatch toast and its dismiss button, and those strings are interpolated into live test names. No dead CSS was found. All 126 class selectors in the two sheets are still rendered, checked in both directions. One thing left as a finding rather than fixed: a sync-rules test named for the loop URL that used to bounce forever now drives `weather boston` through the fallback-engine path, because `weather` is not a command any more. It still passes and still covers something, but not what its name says. Pointing it at a command that does resolve onto an intercepted engine is a behaviour decision, not a comment fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The code indexes arrays constantly and guarded inconsistently: some sites checked the first keyword before reading it, others read a run or an escape plan by index with nothing checking the length. Every one was correct by construction, and nothing was verifying that. Nineteen sites under src/, and not one of them is a non-null assertion. This codebase has no `any`, no ignores and no `!` anywhere in src/, and that is worth more than the shortcut. The fixes are real: a length check written in the form the compiler reads, a regex capture tested instead of its match, a total API in place of an index, and two parallel arrays restructured so the pairing cannot come apart at all. Three sites were correct only because of a fact stated in another file, and each is now local and commented. The sharpest is the escape-rule precondition in the rule fitter: a missing escape rule would have thrown inside the fitter, escaped into the sync, and left the fail-closed branch tearing down the whole dynamic rule table. It now treats that engine as refused, which is what invariant 2 already prescribes. Five branches had to be chosen for cases that cannot arise today. Each takes what the surrounding code does for the nearest case that can: a keyless member drops out of a pack's sample rather than rendering undefined into it, a missing row removes the active-descendant attribute as the no-selection path does, and a character outside the escape map is kept rather than dropped. Under tests/, one helper that throws on a missing element, used sixty-eight times. It never substitutes a default, so a test still fails for the reason it was written to fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ion05
commented
Sep 4, 2026
ion05
commented
Sep 4, 2026
1369 cases across 27 files, down to 150 across 20, running in half a second. The question asked of every survivor was whether a user would notice if it vanished and the code broke. What went. Every design and token assertion, so nothing now enforces the no-literal-hex and type-scale conventions but discipline. Every test that asserted a removed feature stayed removed, which tests history rather than behaviour. The view tests that were DOM assembly rather than decisions, including the jsdom suite added yesterday: the decisions under it are pure and still covered, and the parts that really break need a browser that jsdom cannot be. jsdom leaves package.json with it, since a dependency carrying no tests is worse than neither. The source-text tests, which asserted what the code looked like rather than what it did. And the sweeps: assertions run once per registry row are now single property tests that name every row that drifted. The seventeen invariants were the hard call, because AGENTS.md says their regression tests must never be deleted and that now conflicts with the instruction. Resolved by keeping, for each invariant whose failure a user would meet, the one smallest test that goes red when the bug comes back, with the comment saying which bug that is. Fifteen have one test. Invariant 16 keeps two, because the edit path and the storage boundary are different code and fixing one leaves the other red. Two have none, and AGENTS.md now says so rather than implying cover it never had. The surviving suite was checked by breaking the source three times: the self-interception marker, the pattern that has to swallow Chrome's appended parameters, and the edit that must copy named fields rather than spread. Three, three and five failures respectively. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The crib and the store directory go, along with the listing copy and the padded listing icon. Four things referenced them and are updated rather than left dangling. The README logo now points at the toolbar icon, which is tracked and shipped, so the header still renders; it is the full-bleed art rather than the padded tile, which is the one visible difference. The install section no longer sends a reader to a file that is not there. The release steps in CONTRIBUTING end at uploading the zip. The architecture map drops both entries. The icon generator no longer emits the padded 128px tile, since nothing consumes it now, and the drift check in CI watches only public/icons. Re-running the generator leaves the tracked icons byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The three examples read on their own, and the arrows already say what the sentence was announcing. Co-Authored-By: Claude Opus 5 (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.
Findings from three parallel reviews of the merged v1.1.0 tree: security and privacy, open-source hygiene and licensing, and code quality. Ten commits, each green on its own under typecheck, tests and build.
Bugs found and fixed
applyFilterdecided visibility, so the row was inside adisplay: nonesubtree. Deleting a row had the same hole.wa 42 miles in km, andwaopens WhatsApp.\ghstored a chip that could never match.commitSettingsswallowed failures and did not roll back, leaving the page showing a value that was not in storage.gh facebook/react constructorinterpolatedfunction Object() { [native code] }into the path. Not exploitable, but the shape invariant 17 exists for.Publishing
localStoragefold key it omitted.docs/handoff.mddeleted, with its durable facts folded into AGENTS.md.LICENSEin the shipped zip, a release process, and Dependabot on the Actions pins.Still open
Screenshots do not exist. Nothing in this branch has been exercised in a real browser.