fix linked cards occasionally missing. Use value.data.id #2430

tintinthong · 2025-04-14T12:41:20Z

This PR is to fix the issue where linked data is missing

github-actions · 2025-04-14T12:58:13Z

Host Test Results

1 files ±0 1 suites ±0 29m 37s ⏱️ -58s
916 tests +7 910 ✅ +8 6 💤 ±0 0 ❌ ±0
923 runs +7 917 ✅ +9 6 💤 ±0 0 ❌ - 1

Results for commit b097255. ± Comparison against base commit 04478cd.

This pull request removes 8 and adds 15 tests. Note that renamed tests count towards both.

Chrome ‑ Unit | identity-context garbage collection: a GC candidate is no longer considered a GC candidate if it is consumed by a subscriber
Chrome ‑ Unit | identity-context garbage collection: a GC candidate is no longer considered a GC candidate if it is consumed by an unsaved subscriber
Chrome ‑ Unit | identity-context garbage collection: a GC candidate remains a GC candidate if it is accessed via a set "null" in the identity map by local id
Chrome ‑ Unit | identity-context garbage collection: a GC candidate remains a GC candidate if it is accessed via a set "null" in the identity map by remote id
Chrome ‑ Unit | identity-context garbage collection: an instance becomes a GC candidate when it loses all subscribers
Chrome ‑ Unit | identity-context garbage collection: an instance becomes a GC candidate when it loses all unsaved subscribers
Chrome ‑ Unit | identity-context garbage collection: can mark unsaved instances without subscribers for GC
Chrome ‑ Unit | identity-context garbage collection: can mark unsubscribed instances that have been saved for GC

Chrome ‑ Acceptance | AI Assistant tests: cards are auto-attached in code mode
Chrome ‑ Integration | serialization > linksToMany: can resolve relative URLs in linksToMany fields irrespective of location of linked cards
Chrome ‑ Unit | identity-context garbage collection: a GC candidate is no longer considered a GC candidate if it is consumed by an instance that has a reference count > 0 
Chrome ‑ Unit | identity-context garbage collection: a GC candidate is no longer considered a GC candidate if it is consumed by an unsaved instance that has a reference count > 0
Chrome ‑ Unit | identity-context garbage collection: a GC candidate no longer remains a GC candidate if it accessed via setting a card error in the identity map
Chrome ‑ Unit | identity-context garbage collection: an instance becomes a GC candidate when its reference count drops to 0 for its remote id
Chrome ‑ Unit | identity-context garbage collection: an instance becomes a GC candidate when its reference count drops to zero for its local id
Chrome ‑ Unit | identity-context garbage collection: can add a card error to the identity map
Chrome ‑ Unit | identity-context garbage collection: can delete a card error from the identity map
Chrome ‑ Unit | identity-context garbage collection: can delete an instance from the identity map
…

♻️ This comment has been updated with latest results.

habdelra · 2025-04-14T14:24:15Z

packages/base/card-api.gts

@@ -966,7 +966,11 @@ class LinksTo<CardT extends CardDefConstructor> implements Field<CardT> {
      cachedInstance[isSavedInstance] = true;
      return cachedInstance as BaseInstanceType<CardT>;
    }
-    let resourceId = new URL(value.links.self, relativeTo).href;
+    let relativeId =
+      value.data && Array.isArray(value.data)


this is a LinksTo relationship, not a LinksToMany, why would the value here ever be an array? (keep in mind value here is the JSONAPI serialization for the relationship).

habdelra · 2025-04-14T14:27:56Z

packages/base/card-api.gts

+    let relativeId =
+      value.data && Array.isArray(value.data)
+        ? undefined // we don't know the id of the parent
+        : value.data?.id ?? relativeTo;


generally we strive to serialize with the links.self when sending the serialized form of the instance to the server. both links.self and links.data.id are supported, using both though, can lead to the potential of conflicts if they were ever configured differently from each other. which is why we have opted to pick one (links.self is more concise). i'm super curious what the scenario is when you are deserializing a payload and links.data.id is populated and links.self is not. I think that is the actual bug if this is what is fixing it.

another thought is to remove links.self altogether and purely only use links.data.id (this this also means we should not serialize this so we don't introduce confusion).

and another though is to favor links.self, but fallback to links.data.id if links self is not populated and links.data.id is populated. the drawback is that that gets into the confusing place of having these at odds with one another....

Thanks for the input. The issue is really that value.links.self is relative. In addition, doc.data.id seems like the wrong id to be relative to anyway.

The data values are

value.links.self ../Category/someCategory doc.data.id http://test-realm/test/listing value.data.id http://test-realm/test/Category/someCategory resourceId http://test-realm/Category/someCategory

The expected value

resourceId http://test-realm/test/Category/someCategory

I have added the serialization-test test "can resolve relative URLs in linksToMany fields irrespective of location of linked cards"

another thought is to remove links.self altogether and purely only use links.data.id (this this also means we should not serialize this so we don't introduce confusion).

I think value.data.id seems to return an absolute url which is actually what I want. Perhaps, this is what we want to do.

The JSON API docs consider data to be kinda optional actually. Tho it seems links.self are also treated as optional. I can see why implementation wise it can get unnecessarily complex

I think in deserialization we can prefer value.data.id but I think in general, if we fallback to links.self the deserialization may be wrong

removal of serialization however, lets talk about it perhaps after standup

Yeah, it’s especially tricky too, because I don’t think you can look at your contained card (or contained contained card, etc as the case may be), to see the id of the outermost document that you are relative too. That would be perfect if you could.

Perhaps we need to thread thru a new parameter thru the deserialzation: the id of the document (if there is one), aka “relativeTo” so that we can resolve any relative hrefs. If we did that, there’s about 4-5 places that the relativeTo value would need to be added to function parameters.

If you like maybe we can pair on this after standup.

Yes pls that wud be helpful

I was thinking one argument for not using links.data.id is that it makes it more challenging to copy a set of cards to a different realm. As relative references can be moved more easily.

This reverts commit 8ff063e.

tintinthong force-pushed the fix-linked-cards-occasionally-not-rendering branch from 7df492d to 6a9c51d Compare April 14, 2025 13:36

tintinthong changed the title ~~fix linked cards occasionally missing. Use value.data.id!~~ fix linked cards occasionally missing. Use value.data.id Apr 14, 2025

habdelra reviewed Apr 14, 2025

View reviewed changes

fix not loaded error due to relative

844163d

tintinthong force-pushed the fix-linked-cards-occasionally-not-rendering branch from 6a9c51d to 844163d Compare April 15, 2025 06:15

tintinthong added 5 commits April 15, 2025 14:16

fix lint

16d0b66

remove getDirectoryHref

a70f2ac

fix lint

3b5bcc8

TEST: try to change to use value.data.id

8ff063e

Revert "TEST: try to change to use value.data.id"

b097255

This reverts commit 8ff063e.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix linked cards occasionally missing. Use value.data.id #2430

fix linked cards occasionally missing. Use value.data.id #2430

tintinthong commented Apr 14, 2025 •

edited

Loading

github-actions bot commented Apr 14, 2025 •

edited

Loading

habdelra Apr 14, 2025 •

edited

Loading

habdelra Apr 14, 2025 •

edited

Loading

habdelra Apr 14, 2025

habdelra Apr 14, 2025 •

edited

Loading

tintinthong Apr 15, 2025

tintinthong Apr 15, 2025 •

edited

Loading

habdelra Apr 15, 2025

habdelra Apr 15, 2025 •

edited

Loading

habdelra Apr 15, 2025

tintinthong Apr 15, 2025

habdelra Apr 15, 2025

fix linked cards occasionally missing. Use value.data.id #2430

Are you sure you want to change the base?

fix linked cards occasionally missing. Use value.data.id #2430

Conversation

tintinthong commented Apr 14, 2025 • edited Loading

github-actions bot commented Apr 14, 2025 • edited Loading

Host Test Results

habdelra Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

habdelra Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

habdelra Apr 14, 2025

Choose a reason for hiding this comment

habdelra Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

tintinthong Apr 15, 2025

Choose a reason for hiding this comment

tintinthong Apr 15, 2025 • edited Loading

Choose a reason for hiding this comment

habdelra Apr 15, 2025

Choose a reason for hiding this comment

habdelra Apr 15, 2025 • edited Loading

Choose a reason for hiding this comment

habdelra Apr 15, 2025

Choose a reason for hiding this comment

tintinthong Apr 15, 2025

Choose a reason for hiding this comment

habdelra Apr 15, 2025

Choose a reason for hiding this comment

tintinthong commented Apr 14, 2025 •

edited

Loading

github-actions bot commented Apr 14, 2025 •

edited

Loading

habdelra Apr 14, 2025 •

edited

Loading

habdelra Apr 14, 2025 •

edited

Loading

habdelra Apr 14, 2025 •

edited

Loading

tintinthong Apr 15, 2025 •

edited

Loading

habdelra Apr 15, 2025 •

edited

Loading