Skip to content

fix: correct two example bugs caught by skills#70's review#589

Open
kriszyp wants to merge 1 commit into
mainfrom
kris/fix-v5-upgrade-await-resellerids
Open

fix: correct two example bugs caught by skills#70's review#589
kriszyp wants to merge 1 commit into
mainfrom
kris/fix-v5-upgrade-await-resellerids

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 16, 2026

Copy link
Copy Markdown
Member

Summary

  • release-notes/v5-lincoln/v5-migration.md: the polling-loop example was missing await on delay(100) — since delay is imported from node:timers/promises, the un-awaited call turned the intended ~10Hz poll into a tight loop that hammers Table.get as fast as the read path allows.
  • reference/rest/querying.md: the resellers relationship example pointed from at the singular resellerId, which the schema never declares — the indexed field is plural resellerIds. A relationship built from this example can't resolve its foreign key.

Both found by a cross-model (Codex) review on HarperFast/skills#70, which regenerates its rules from this repo — this PR fixes the source so a future regen picks up the correction.

A third finding from that review — recommending allowReadRecord over allowRead for record-scoped authorization in the vector-indexing guide — was not applied: allowReadRecord doesn't exist in Harper's actual codebase, and a synchronous allowRead override already is the correct record-scoped hook (confirmed against resources/Resource.ts). That doc example was correct as written.

Test plan

  • Diffed against the exact commit skills#70 was regenerated from (c5be2f9) to confirm both fixes land in the right source location.
  • Verified delay is a real node:timers/promises import (returns a Promise) in both affected code blocks.
  • Verified the schema block only declares resellerIds (plural), confirming resellerId in the relationship directive was a typo, not an intentional alias.
  • Docs-only change; no build/test suite applicable beyond the above manual verification.

- v5-migration.md: the polling-loop example was missing await on
  delay(100), turning the intended ~10Hz poll into a tight loop
  (delay() returns a promise from node:timers/promises).
- querying.md: the resellers relationship example pointed `from` at
  the singular `resellerId`, which the schema never declares — the
  indexed field is plural `resellerIds`.

Both found by a cross-model review on HarperFast/skills#70 (which
regenerates its rules from this repo). A third finding from that
review — recommending `allowReadRecord` over `allowRead` for
record-scoped authorization — was not applied: `allowReadRecord`
doesn't exist in Harper; a synchronous `allowRead` override already is
the correct record-scoped hook, so that example was correct as written.
@kriszyp
kriszyp requested a review from a team as a code owner July 16, 2026 01:51

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request corrects a relationship field reference in the REST querying documentation and adds missing await keywords to delay calls in the v5 migration guide examples. The review feedback correctly identifies a missing import for transaction in one of the migration guide code examples, which should be added to make the example fully functional.

// we could also explicitly start a new transaction here for each get:
while ((await transaction(() => Table.get(target))).status !== 'ready') {
delay(100);
await delay(100);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In this code example, transaction is used on line 95 but is not imported from 'harper' on line 87 (which only imports getContext). To ensure the example is fully functional and copy-pasteable, please update the import statement on line 87 to include transaction:

import { getContext, transaction } from 'harper';

@github-actions

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-589

This preview will update automatically when you push new commits.

@heskew heskew left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two fixes are right (resellerIds resolves the FK mismatch; await delay(100) fixes both polling loops). One gap remains in this same example — left inline.

🤖 Draft prepared with Claude (cross-model review pipeline); reviewed and posted by @heskew.

@@ -93,7 +93,7 @@ class MyResource {
// now we can call Table.get and it will read the latest data.
// we could also explicitly start a new transaction here for each get:
while ((await transaction(() => Table.get(target))).status !== 'ready') {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example calls transaction(...) but it isn't imported — the import on line 87 only brings in getContext, so the snippet isn't copy-pasteable as written (same gap gemini flagged). Suggest:

import { getContext, transaction } from 'harper';

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.

3 participants