ENG-1869 Review and accept imported relation instances in Roam - #1384
ENG-1869 Review and accept imported relation instances in Roam#1384sid597 wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
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. |
PR size/scope checkThis PR is over our review-size guideline.
Please split this into smaller PRs unless there is a clear reason the changes need to land together. If keeping it as one PR, please add a brief justification covering:
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![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: eec212b386
ℹ️ 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".
| const tentativeKeys = new Set( | ||
| (await getTentativeRelationInstances()).map( | ||
| (t) => `${t.schemaUid}|${t.sourceUid}|${t.destinationUid}`, | ||
| ), |
There was a problem hiding this comment.
When a page contains many mounted discourse-context overlay buttons, every button's getInfo invokes this path, which calls getTentativeRelationInstances; that helper scans the global relations page and synchronously pulls block props for every tentative relation. Rendering N overlay badges with M imports therefore adds N global scans and N×M pulls even though this path only needs identity keys, which can make ordinary page rendering and score calculation sluggish in larger graphs; query the tentative keys directly or cache/index them rather than loading full provenance each time.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deliberately left uncached for now: getReifiedRelations is a single datalog query over the children of one page, and it rides alongside the much heavier relation fireQuery each overlay button already runs. A cache would need invalidation wired through the accept/remove/create refresh paths, which is more risk than the query costs today. Happy to memoize next to resultCache if profiling on a large graph shows it matters.
| const SANE_ROLE_NAME_RE = new RegExp(/^[\w\-]*$/); | ||
| // Annotations describe a relation's review/provenance state; they are not part | ||
| // of its identity, so lookups by role parameters must ignore them. | ||
| const RELATION_ANNOTATION_KEYS = new Set<string>([ |
There was a problem hiding this comment.
Lookups by role parameters now ignore tentative and importedFrom. Without this, the delete action in ResultsTable.onDelete can never find an accepted imported relation: it queries with 3 role params against a block carrying 4 or 5 prop keys. A block with a genuine extra role key (e.g. contextUid) is still rejected; covered by the "still rejects blocks with extra role keys" test.
| const existing = await strictQueryForReifiedBlocks(data); | ||
| if (existing !== null) return existing; | ||
| if (existing !== null) { | ||
| if (parameterUids[TENTATIVE_PROP_KEY] === undefined) { |
There was a problem hiding this comment.
A local create that matches an existing tentative import clears the tentative flag rather than returning the block untouched: explicit local creation is acceptance. Without this, the create dialog, suggestions accept, and canvas relation draw would all report success while the relation stayed hidden as pending review. The import path passes tentative: true, so re-import does not promote.
| resultsWithRelation.length > 0 && | ||
| resultsWithRelation[0].results.length > 0 | ||
| ) { | ||
| const tentativeKeys = new Set( |
There was a problem hiding this comment.
Tentative instances are excluded here rather than in the shared datalog translator, because the translator's basis reads only sourceUid/destinationUid/hasSchema (ENG-1867, PR #1302). Filtering here also covers deriveDiscourseNodeAttribute, which routes through this function, so the overlay score stays consistent with the tab counts.
|
Size check ack: 647 lines includes roughly 330 lines of unit tests. The non-test change is one coupled unit: the pending-review section, the exclusion of tentative instances from accepted results, and the |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| const loadRows = useCallback(async () => { | ||
| if (!getStoredRelationsEnabled()) return; | ||
| const instances = await getTentativeRelationInstances(); | ||
| const relevant = instances.filter( | ||
| (instance) => | ||
| instance.sourceUid === uid || instance.destinationUid === uid, | ||
| ); | ||
| const relationById = new Map(getDiscourseRelations().map((r) => [r.id, r])); | ||
| const nextRows = relevant.map((instance) => { | ||
| const isOutgoing = instance.sourceUid === uid; | ||
| const otherUid = isOutgoing | ||
| ? instance.destinationUid | ||
| : instance.sourceUid; | ||
| const schema = relationById.get(instance.schemaUid); | ||
| const label = | ||
| (isOutgoing ? schema?.label : schema?.complement || schema?.label) || | ||
| "Unknown relation"; | ||
| return { | ||
| ...instance, | ||
| label, | ||
| otherText: getPageTitleByPageUid(otherUid) || otherUid, | ||
| provenance: buildProvenance(instance.importedFrom), | ||
| }; | ||
| }); | ||
| setRows(nextRows); | ||
| onCountChange(nextRows.length); | ||
| }, [uid, onCountChange]); | ||
|
|
||
| useEffect(() => { | ||
| void loadRows(); | ||
| }, [loadRows]); |
There was a problem hiding this comment.
Unhandled promise rejections in async operations. The loadRows function is called with void (lines 83, 86), which prevents error handling. If getTentativeRelationInstances() or any other async operation fails, it will cause an unhandled promise rejection that could crash the application or leave the UI in a broken state.
const loadRows = useCallback(async () => {
if (!getStoredRelationsEnabled()) return;
try {
const instances = await getTentativeRelationInstances();
// ... rest of the logic
} catch (error) {
internalError({
error,
type: "Load Tentative Relations Failed",
context: { uid },
sendEmail: false,
});
setRows([]);
onCountChange(0);
}
}, [uid, onCountChange]);| const loadRows = useCallback(async () => { | |
| if (!getStoredRelationsEnabled()) return; | |
| const instances = await getTentativeRelationInstances(); | |
| const relevant = instances.filter( | |
| (instance) => | |
| instance.sourceUid === uid || instance.destinationUid === uid, | |
| ); | |
| const relationById = new Map(getDiscourseRelations().map((r) => [r.id, r])); | |
| const nextRows = relevant.map((instance) => { | |
| const isOutgoing = instance.sourceUid === uid; | |
| const otherUid = isOutgoing | |
| ? instance.destinationUid | |
| : instance.sourceUid; | |
| const schema = relationById.get(instance.schemaUid); | |
| const label = | |
| (isOutgoing ? schema?.label : schema?.complement || schema?.label) || | |
| "Unknown relation"; | |
| return { | |
| ...instance, | |
| label, | |
| otherText: getPageTitleByPageUid(otherUid) || otherUid, | |
| provenance: buildProvenance(instance.importedFrom), | |
| }; | |
| }); | |
| setRows(nextRows); | |
| onCountChange(nextRows.length); | |
| }, [uid, onCountChange]); | |
| useEffect(() => { | |
| void loadRows(); | |
| }, [loadRows]); | |
| const loadRows = useCallback(async () => { | |
| if (!getStoredRelationsEnabled()) return; | |
| try { | |
| const instances = await getTentativeRelationInstances(); | |
| const relevant = instances.filter( | |
| (instance) => | |
| instance.sourceUid === uid || instance.destinationUid === uid, | |
| ); | |
| const relationById = new Map(getDiscourseRelations().map((r) => [r.id, r])); | |
| const nextRows = relevant.map((instance) => { | |
| const isOutgoing = instance.sourceUid === uid; | |
| const otherUid = isOutgoing | |
| ? instance.destinationUid | |
| : instance.sourceUid; | |
| const schema = relationById.get(instance.schemaUid); | |
| const label = | |
| (isOutgoing ? schema?.label : schema?.complement || schema?.label) || | |
| "Unknown relation"; | |
| return { | |
| ...instance, | |
| label, | |
| otherText: getPageTitleByPageUid(otherUid) || otherUid, | |
| provenance: buildProvenance(instance.importedFrom), | |
| }; | |
| }); | |
| setRows(nextRows); | |
| onCountChange(nextRows.length); | |
| } catch (error) { | |
| internalError({ | |
| error, | |
| type: "Load Tentative Relations Failed", | |
| context: { uid }, | |
| sendEmail: false, | |
| }); | |
| setRows([]); | |
| onCountChange(0); | |
| } | |
| }, [uid, onCountChange]); | |
| useEffect(() => { | |
| void loadRows(); | |
| }, [loadRows]); | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
| ) : ( | ||
| <div className="flex flex-col items-start"> | ||
| <span>No discourse relations found.</span> | ||
| {!tentativeCount && <span>No discourse relations found.</span>} |
There was a problem hiding this comment.
Race condition: tentativeCount starts at 0 and is only updated asynchronously after TentativeRelationInstances mounts and loads data. This causes "No discourse relations found." to briefly flash on screen even when tentative relations exist, then disappear once setTentativeCount is called.
// Fix: Initialize with undefined to indicate loading state
const [tentativeCount, setTentativeCount] = useState<number | undefined>(undefined);
// Then update the condition:
{tentativeCount === 0 && <span>No discourse relations found.</span>}Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const numParams = countRoleKeys(parameterUids); | ||
| const resultF = result | ||
| .filter(([, params]) => Object.keys(params).length === numParams) | ||
| .filter(([, params]) => countRoleKeys(params) === numParams) |
There was a problem hiding this comment.
🟡 Deletion targets the wrong relation
When accepted and pending copies share endpoints and type, strictQueryForReifiedBlocks returns both in unspecified order. Deletion can remove the pending copy while leaving the selected accepted relation visible.
Prompt for agents
Update relation lookup and deletion so annotation-insensitive deduplication does not make an accepted relation and its tentative twin interchangeable. `strictQueryForReifiedBlocks` in apps/roam/src/utils/createReifiedBlock.ts now returns both blocks for an unannotated identity query, while `ResultsTable.onDelete` in apps/roam/src/components/results-view/ResultsTable.tsx deletes the first result. Preserve annotation-insensitive matching for creation, but let deletion select the accepted block deterministically. Add coverage for deleting a visible accepted relation when a tentative twin exists.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Reviewer brief
tentativeflag in place (provenance kept, no new block); remove deletes the reified block. Failures report throughinternalError(PostHog + toast) and leave the relation tentative. The section is gated ongetStoredRelationsEnabled().strictQueryForReifiedBlocksignores thetentativeandimportedFromannotation keys when matching role parameters; without this, an accepted imported relation (4 prop keys) could never be found by the existing delete action, and local creation would duplicate an imported relation instead of deduping. (2) A local create whose dedupe hit is a tentative import promotes it, best-effort (explicit local creation counts as acceptance); otherwise the create dialog, suggestions, and canvas would report success while the relation stayed hidden as pending review. This also meansmigrateRelationspromotes a tentative import that matches a legacy local relation, which is intentional: the local relation already existed.publishNodesToGroupsfilter onimportedFromRidis a no-op in Roam (nothing writes that key), so imported relations, including unaccepted ones, are currently publishable; pre-existing ENG-1867 gap, follow-up ticket drafted. Chain coverage: this PR handles Roam-side instance acceptance only; Obsidian-side instance acceptance already exists inRelationshipSection.tsx, and type/triple acceptance is ENG-2131.Verification
pnpm install --frozen-lockfileandpnpm ci:validatepass from the repo root (check-types across all packages; 134 roam unit tests).getDiscourseContextResults(both directions), accepted-twin visibility ingetTentativeOnlyRelationKeys, accept promote/no-op/failure, local-create promotion vs re-import non-promotion increateReifiedRelation, and annotation-awarestrictQueryForReifiedBlocks.Loom video
eng-1869-20260907.mp4
Scope check
$scope-checkagainst ENG-1869 and the final diff.Done When:strictQueryForReifiedBlockstreatstentativeandimportedFromas annotation keys when counting role parameters, and a local create that matches a tentative import promotes it. Both change relation lookup/creation behavior outside the overlay UI.importedFromon the block, so without these the promoted relation cannot be found by the existing delete action and local creation either duplicates the imported relation or silently returns a hidden one.Local delegated full review