Repository navigation
Harden capture: Readability, item index, Reader updates, toast retry - #2
Conversation
Keep saving locally when the LLM fails, stop dropping CJK tags, and avoid treating page-injected tags as HTML. Also align extraction with the isolated world, stop double-counting Pinboard imports, drop unused permissions, and add CI for typecheck/tests. Co-authored-by: i.an <randomradio@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6bba58279
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (root) { | ||
| const clone = root.cloneNode(true) as HTMLElement; | ||
| clone.querySelectorAll('script, style, noscript, nav, footer, aside, iframe, [role="navigation"]').forEach((el) => el.remove()); | ||
| text = (clone.innerText || '').replace(/\n{3,}/g, '\n\n').trim(); |
There was a problem hiding this comment.
When the bundled Readability stub is used, this reads innerText from a detached clone. Detached elements are not rendered, so innerText falls back to descendant text content rather than applying the page's rendered visibility; CSS-hidden text inside an article or main can therefore enter the excerpt and be sent to the configured LLM. Derive visibility from the connected DOM before cloning/removing unwanted elements.
Useful? React with 👍 / 👎.
| try { | ||
| if (!tab?.id) throw new Error('No active tab'); | ||
| const settings = await getSettings(); | ||
| await ensureOriginPermission(settings.llm.baseUrl); |
There was a problem hiding this comment.
On a first toolbar capture for an LLM origin that has not been granted, declining the optional-host permission makes ensureOriginPermission throw before extraction and captureAndSync, so the page is not saved locally or sent to configured Pinboard/Readwise destinations. This bypasses the new pipeline behavior that records LLM failures while continuing the capture; treat a denied LLM permission as an LLM destination error rather than aborting the entire toolbar action.
Useful? React with 👍 / 👎.
| } catch { | ||
| statusEl.textContent = 'Advanced config must be valid JSON.'; | ||
| return; |
There was a problem hiding this comment.
If a user enters a new LLM origin and invalid advanced JSON, the code grants permission for the new origin first and then returns here without saving the settings or removing that permission. The extension is left with unused host access that is no longer represented by its saved LLM origin; parse and validate the form before requesting permission, or revoke the newly granted permission on this failure path.
Useful? React with 👍 / 👎.
Bundle Mozilla Readability for extraction, look up saved items through an index instead of scanning local storage, create-or-update Readwise documents when they already exist, and move the capture toast into a content script that can retry destinations and edit tags. Co-authored-by: i.an <randomradio@users.noreply.github.com>
cursor
Bot
changed the title
Cover each exported helper in background, common, toast, and settings preview code, including LLM, Pinboard, Readwise, exporters, and storage. Co-authored-by: i.an <randomradio@users.noreply.github.com>
setup-node cache: pnpm runs `pnpm store path`, which pnpm 9 rejects when pnpm-workspace.yaml has only allowBuilds. Declare the root package and run CI on pnpm 10 to match local. Co-authored-by: i.an <randomradio@users.noreply.github.com>
Keep main's save-on-capture toggle and this branch's destination retry, toast, and unit tests. Pinboard skips when capture is off. Co-authored-by: i.an <randomradio@users.noreply.github.com>
What changed
Follow-up to the review in this PR. The four remaining high-value items are now implemented, plus unit tests for every exported function.
Merged latest
main(feat: make Pinboard and Readwise capture optional). Conflicts were complementary: this branch keeps retry/toast/Readability, and Pinboard now honors the save-on-capture toggle.1. Real Readability
@mozilla/readabilityasdist/readability.jsand attacheswindow.Readabilityin the isolated world.article/main, thendocument.body.innerText.2. Indexed local items
findItemByUrl/listItemsuse anitemIndex(ids+byUrl) instead ofchrome.storage.local.get(null).item:records are migrated once if no index exists.3. Readwise create-or-update
POST /api/v3/save/still creates the document.200(already exists) nowPATCH /api/v3/update/<id>/with title, summary, and tags.readwiseDocumentId, later saves and tag edits PATCH directly.4. Content-script toast
content/toast.js(not an inlineexecuteScriptfunction).5. Unit tests for each exported function
preview-draft,toast-helpers,options-helpers) so they can be tested without Chrome/DOM bootstrapping.6. CI
CI / testfailed inactions/setup-node(cache: pnpm) withpackages field missing or empty.pnpm-workspace.yamlnow declarespackages: ['.'], and CI/release use pnpm 10 sopnpm store pathworks withallowBuilds.Earlier review fixes still in this PR
notifications/alarmspermissions removed.Tests
pnpm typecheckpnpm test— 136 passingpnpm buildManual check after loading
dist/I could not load the unpacked extension in this environment’s browser; verification here is tests + build.