fix(browse): handle rows with an undefined primary key (#1199)#1529
Open
dawsontoth wants to merge 3 commits into
Open
fix(browse): handle rows with an undefined primary key (#1199)#1529dawsontoth wants to merge 3 commits into
dawsontoth wants to merge 3 commits into
Conversation
When a table's @PrimaryKey is changed on a populated table, Harper does not re-key the existing rows, so `describe` reports a primary key attribute the old rows have no value for. Clicking such a row sent `ids: [null]` to search_by_id (JSON.stringify turns `[undefined]` into `[null]`), which Harper rejects with a 500 'hash_values' must be strings or numbers — leaving the edit modal spinning on a loading spinner forever. - onRowClick detects the missing primary key and shows the row read-only using the data the list query already returned, instead of firing a doomed lookup. - EditTableRowModal shows a warning banner explaining the row can't be looked up, edited, or deleted individually, and hides the Save/Delete actions. - getSearchByIdOptions filters null/undefined ids so a null hash value is never sent (belt-and-suspenders for any caller). - Delete guard uses `!= null` so a record with a falsy-but-valid key (0, "") can still be deleted. The root-cause data-loss bug (a `delete` with a null hash value wiped the whole table in Harper) is fixed in HarperFast/harper#1837. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 16, 2026
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where rows with missing primary key values could not be viewed or managed in the database table view. The changes introduce logic to detect missing primary keys, stash the row data locally, and render the edit modal in a read-only state with an informative warning. Additionally, the getSearchByIdOptions utility and its corresponding tests were updated to filter out null or undefined IDs, preventing invalid API requests that previously caused server errors. I have no feedback to provide as there were no review comments.
Covers the new behavior surfaced in the coverage report: with a missing primary key the modal warns, goes read-only, and hides Save/Delete; a normal row still edits and deletes. Monaco is stubbed since it can't load in jsdom. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cb1kenobi
reviewed
Jul 16, 2026
…rimary key (#1199) A table whose primary key was changed after rows existed reports one primary key via describe but is still keyed by the original attribute. Two consequences in the edit modal, both now handled by falling back to the row the list already gave us: - The edit fetch hardcoded `onlyIfCached: true`; on a key miss Harper answers `{message: "Entry is not cached"}`, which the modal rendered as the record. It now sends `onlyIfCached: false`, so a genuine miss returns an empty result and a real record loads for editing regardless of cache state. - A row whose declared primary-key value exists but resolves to no stored record (e.g. `email` present but the table is really keyed by `id`) now shows a read-only "couldn't be loaded" banner instead of a raw error/empty editor. The underlying Harper bug (changing @PrimaryKey on a populated table leaves the real hash attribute unchanged, so describe reports the wrong key) is fixed in HarperFast/harper#1837. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dawsontoth
added a commit
to HarperFast/harper
that referenced
this pull request
Jul 16, 2026
Changing which attribute is the `@primaryKey` (via GraphQL schema, defineTable, or create_table) on a table that already contains records left the table corrupt: the storage/hash key (`Table.primaryKey`) was never re-pointed, so `describe_table` began reporting the newly-declared attribute while every record — old and newly inserted, even via the raw Operations API — stayed keyed by the original attribute. search_by_id, update, and delete by the declared key then all miss. `table()` is the single factory every front-end funnels through, so guard there: on the existing-table branch, if a schema-authored declaration names a different primary key and the table has any records, throw a ClientError instead of silently diverging describe from storage. Empty tables and settings-only reasserts are unaffected. This is the root cause behind HarperFast/studio#1199 (the studio side handles the resulting orphaned rows defensively in HarperFast/studio#1529). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kriszyp
pushed a commit
to HarperFast/harper
that referenced
this pull request
Jul 17, 2026
Changing which attribute is the `@primaryKey` (via GraphQL schema, defineTable, or create_table) on a table that already contains records left the table corrupt: the storage/hash key (`Table.primaryKey`) was never re-pointed, so `describe_table` began reporting the newly-declared attribute while every record — old and newly inserted, even via the raw Operations API — stayed keyed by the original attribute. search_by_id, update, and delete by the declared key then all miss. `table()` is the single factory every front-end funnels through, so guard there: on the existing-table branch, if a schema-authored declaration names a different primary key and the table has any records, throw a ClientError instead of silently diverging describe from storage. Empty tables and settings-only reasserts are unaffected. This is the root cause behind HarperFast/studio#1199 (the studio side handles the resulting orphaned rows defensively in HarperFast/studio#1529). Co-Authored-By: Claude Opus 4.8 <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
2 participants
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.
Closes #1199.
The state (yes, it's reproducible)
A record can end up with no value for its declared primary key. The path: change a table's
@primaryKeyon a populated table (or formalize a previously-dynamic table with a natural key, e.g.id: ID→email: String @primaryKey, then redeploy). Harper accepts the change but does not re-key the existing rows — they keep their original stored key (the oldid), anddescribenow reports the new primary-key attribute the old rows have no value for.So
rowData.original[primaryKey]isundefined. Clicking the row did:…and the edit modal just spun on
<Loading />forever (the query hasretry: falseand no error toast).What this PR does
onRowClickdetects the missing primary key, skips the doomedsearch_by_id, and opens the modal on the row the list query already returned (answering the issue's "does it even need to re-fetch?" — no).EditTableRowModalshows a warning banner ("This row has no primary key value… usually means the table's primary key was changed after the row was created"), renders the row read-only, and hides the Save/Delete actions.getSearchByIdOptionsfiltersnull/undefinedids so a null hash value is never sent (belt-and-suspenders for any caller).!= nullinstead of a truthy check, so a record with a falsy-but-valid key (0,"") can still be deleted.Unit test added for the id filtering; full suite (1688 tests) green.
The scarier thing I found
While tracing the delete path I found that a Harper
deletewithhash_values: [null]deletes the entire table while reporting"1 of 1 record successfully deleted"(verified on 4.7.23 and 5.0.15). Studio's delete button happens to guard against it, but that's core data-loss — fixed here: HarperFast/harper#1837.Full investigation and reproduction steps: #1199.
🤖 Generated with Claude Code