Skip to content

ENG-2167 nodeSchemaToCrossApp is missing modifiedAt - #1335

Open
maparent wants to merge 1 commit into
mainfrom
eng-2167-nodeschematocrossapp-is-missing-modifiedat
Open

ENG-2167 nodeSchemaToCrossApp is missing modifiedAt#1335
maparent wants to merge 1 commit into
mainfrom
eng-2167-nodeschematocrossapp-is-missing-modifiedat

Conversation

@maparent

@maparent maparent commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

@vercel

vercel Bot commented Aug 23, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
discourse-graph Ready Ready Preview Aug 23, 2026 5:46pm

Request Review

@linear-code

linear-code Bot commented Aug 23, 2026

Copy link
Copy Markdown

ENG-2167

@supabase

supabase Bot commented Aug 23, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@devin-ai-integration devin-ai-integration 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.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@maparent
maparent force-pushed the eng-2167-nodeschematocrossapp-is-missing-modifiedat branch from e2b4f6b to 77fde60 Compare August 23, 2026 15:07
if (!relData) return null;
const userUid = (relData[":create/user"] ?? {})[":user/uid"];
if (!userUid) return null;
const createdTime = relData[":create/time"] || Date.now();

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.

Inconsistent operator usage creates a bug with timestamp value 0. Line 199 uses || (OR operator) while lines 203-204 use ?? (nullish coalescing). If :create/time is 0 (Unix epoch, a valid timestamp), the || operator treats it as falsy and incorrectly falls back to Date.now(), creating an incorrect creation time. This is inconsistent with the nullish coalescing used elsewhere.

Fix:

const createdTime = relData[":create/time"] ?? Date.now();

This ensures only null or undefined trigger the fallback, not the valid timestamp 0.

Suggested change
const createdTime = relData[":create/time"] || Date.now();
const createdTime = relData[":create/time"] ?? Date.now();

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@maparent
maparent requested review from mdroidian and sid597 August 23, 2026 15:15
@maparent
maparent force-pushed the eng-2167-nodeschematocrossapp-is-missing-modifiedat branch from 77fde60 to 5579a4e Compare August 23, 2026 17:45
@mdroidian

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 5579a4ef92

ℹ️ 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".

s: DiscourseNode,
): CrossAppNodeSchema | null => {
const relData = window.roamAlphaAPI.pull(
"[:create/time :edit/time {:create/user [:user/uid]}]",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does :page/edit-time not track both blocks+page edit time?

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.

It does. cc @maparent

s: DiscourseNode,
): CrossAppNodeSchema | null => {
const relData = window.roamAlphaAPI.pull(
"[:create/time :edit/time {:create/user [:user/uid]}]",

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.

It does. cc @maparent

if (!userUid) return null;
const createdTime = relData[":create/time"] || Date.now();
// A node type's settings live either in the page's props or in blocks below it,
// depending on the settings store in use, so neither time alone sees every edit:

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.

page/edit-time will see both

authorId: userUid,
createdAt: new Date(relData[":create/time"] || Date.now()),
createdAt: new Date(createdTime),
modifiedAt: new Date(Math.max(editTime, pageEditTime, createdTime)),

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.

nit: probably just need pageEditTime

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