Skip to content

zotero-roam-mcp: quality and correctness pass on the AI tools - #23

Open
mattakamatsu wants to merge 1 commit into
mainfrom
zotero-roam-mcp
Open

zotero-roam-mcp: quality and correctness pass on the AI tools#23
mattakamatsu wants to merge 1 commit into
mainfrom
zotero-roam-mcp

Conversation

@mattakamatsu

Copy link
Copy Markdown
Contributor

Follow-up to #22 (merged). Single commit, confined to the zotero-roam-mcp prototype, from a review pass on the AI-tools code before offering it upstream to 8bitgentleman/zotero-roam.

Quality (reuse, simplification, efficiency, altitude)

  • Use the extension's existing getItemYear helper instead of a local copy of the same ternary.
  • Share the settings derivation with configRoamDepot, as a new getCurrentSettings in setup.ts, so "the extension's current settings" is defined in one place.
  • Resolve inGraph with a single getCitekeyPages() query instead of one findRoamPage per result (that was up to 50 datalog round-trips per search).
  • Merge the citekey lookup and children identification into one findItemWithChildren; share the import-outcome epilogue between the two import tools.
  • Trim the test extension-API helper to what the tools actually use, and dedupe the two identical mock import outcomes.

Correctness

Found by review, each verified against the real code:

  • Raw TypeError on bad input. Handlers are callable directly from JS, where Roam's schema validation does not apply, so {citekey: null} produced Cannot read properties of null (reading 'trim') instead of one of the instructive messages. Arguments are now coerced.
  • An uncertain outcome was reported as success. addBlocksArray resolves with success: null (never false) when the formatted output is empty, after creating the page. An agent calling zotero-import-metadata with a custom formatter that returns nothing got success: null and no error, leaving an empty ghost page behind while reporting success. Anything short of a confirmed import now throws, and a non-Error failure reason is folded into the message instead of discarded.
  • Stale citekey map after a headless import. Every UI import path refreshes RoamCitekeysContext when it creates a page; the AI tools could not, since that map is React state. The UI would then still consider the page missing, and a later "Import metadata" click would try to create a page whose title already exists. The provider now refreshes on the extension's own metadata-added / notes-added events, which fixes the staleness for any non-UI importer, not just these tools.
  • Case-sensitivity mismatch. Search matched citekeys case-insensitively, import did not, so a citekey transcribed with different casing failed with a message that pointed the agent at the wrong causes.
  • Experimental API could break the extension. addTool throwing in onload would have prevented render(); removeTool throwing in onunload would have skipped unmount and deleteDatabase(). Both calls are now guarded.
  • Search gained offset paging (previously an agent could see total: 185 with no way to reach past the first 50), and a failed registration clears the stored API handle.

Tests

38 tests in the service (up from 25). The failure paths were previously unverified: deleting the entire throw block from the outcome handling left every test green. They now cover failed, non-Error, and empty-output imports, argument coercion, offset paging, case-insensitive lookup, and the registration lifecycle.

pnpm test passes 444/444 with no type errors; pnpm build and pnpm prepare:artifacts are clean.

🤖 Generated with Claude Code

Quality pass (reuse/simplification/efficiency/altitude):
- use the existing getItemYear helper instead of a local copy
- share the settings derivation with configRoamDepot as setup.ts's
  getCurrentSettings
- resolve inGraph with one getCitekeyPages() query instead of one
  findRoamPage per result
- merge the citekey lookup and children identification into
  findItemWithChildren; share the import-outcome epilogue
- trim the test extension-API helper and dedupe the mock import outcomes

Correctness fixes:
- coerce handler arguments (handlers are callable directly from JS, where
  Roam's schema validation does not apply), so a bad citekey or query
  yields an instructive message rather than a raw TypeError
- treat an uncertain outcome (success: null, which leaves an empty page)
  as a failure, and fold non-Error failure reasons into the message
- match citekeys case-insensitively on import, as search already did
- refresh the citekey-page map when an import creates a page, so headless
  imports cannot leave the UI believing the page is missing
- add offset paging to the search tool
- guard the experimental AI-tools calls in onload/onunload, and clear the
  stored API handle when registration fails

Adds tests for the failure paths, which previously passed with the throw
removed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Roam prototype previews

The preview deployment is ready. Paste a URL below into Load Developer Extensions from URL in Roam:

View publishing details

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant