"feat: integrate @solid/object and @volunteeringdata/object libraries by PreciousOritsedere · Pull Request #10 · theodi/volunteering-demo

PreciousOritsedere · 2026-05-19T08:37:11Z

Replace custom Agent with extension of @solid/object's Agent
Remove redundant vocabulary constants handled by the library
Delete WebIdDataset (no longer needed)
Simplify profileUtils to use Agent constructor directly
Replace wrapVolunteerProfile with VolunteerProfile constructor
Add VCardValueNode workaround for @solid/object HasValue infinite recursion bug
Add safeGet wrapper for graceful handling of Pod data mismatches
Add structuralSharing: false for React Query Agent caching"

- Replace custom Agent with extension of @solid/object's Agent - Remove redundant vocabulary constants handled by the library - Delete WebIdDataset (no longer needed) - Simplify profileUtils to use Agent constructor directly - Replace wrapVolunteerProfile with VolunteerProfile constructor - Add VCardValueNode workaround for @solid/object HasValue infinite recursion bug - Add safeGet wrapper for graceful handling of Pod data mismatches - Add structuralSharing: false for React Query Agent caching"

vercel · 2026-05-19T08:37:16Z

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

Project	Deployment	Actions	Updated (UTC)
profile-manager-demo	Ready	Preview, Comment	May 29, 2026 11:25am

langsamu · 2026-05-19T10:31:13Z

+export class Agent extends BaseAgent {
+  /**
+   * Override email to avoid @solid/object's HasValue class which has a
+   * .value getter that conflicts with N3's termToId (causes infinite recursion).


See solid/object#30 instead.

See solid/object#30 instead.

Thanks for the fix in solid/object#30! The published npm version (0.5.0) still has the issue. Happy to remove it once a new release is published, could you cut a new release of the package with the fix please?

https://www.npmjs.com/package/@solid/object/v/0.6.0

https://www.npmjs.com/package/@solid/object/v/0.6.0

I have implemented this, please see 0ead6f2

langsamu · 2026-05-19T10:32:33Z

  get email(): string | null {
-    return this.hasEmail?.actualValue ?? null;
+    const emailNode = this.singularNullable(VCARD.hasEmail, TermAs.instance(VCardValueNode));
+    if (!emailNode) return null;


Is there a good readon to return null? Otherwise let them all be undefined if they are optional.

@langsamu The base Agent class in the published @solid/object@0.5.0 defines email and phone as string | null, so the override has to return null to match. Once #30 is released to npm, I'll remove these overrides completely and just use the library's email/phone directly.

this is also done — upgraded to @solid/object@0.6.0. The email/phone overrides and VCardValueNode workaround are removed entirely. see the commit here

langsamu · 2026-05-19T10:34:00Z

-    return (addr != null && addr !== "") ? addr : null;
+    return addr != null && addr !== "" ? addr : null;


This change does not belong in this PR.

Fixed — reverted the formatting change. See the commit here

langsamu · 2026-05-19T10:37:08Z

+/**
+ * Extended Agent — adds project-specific properties on top of @solid/object's Agent.
+ *
+ * The base Agent from @solid/object already provides:
+ *   vcardFn, foafName, name, email, hasEmail, phone, hasTelephone,
+ *   organization, role, title, website, vcardHasUrl, foafHomepage,
+ *   photoUrl, storageUrls, pimStorage, solidStorage, oidcIssuer, knows
+ *
+ * We extend only for properties not yet in the shared library.
+ */
+


Please remove all comments that hinder readability and maintainability by redundantly explaining what the code does instead of why it does it.

Removed. However, I only kept the comment explaining why VCardValueNode exists since that's not obvious from the code alone. See the commit

langsamu · 2026-05-19T10:38:37Z

+/**
+ * Extended Agent — adds project-specific properties on top of @solid/object's Agent.
+ *
+ * The base Agent from @solid/object already provides:
+ *   vcardFn, foafName, name, email, hasEmail, phone, hasTelephone,
+ *   organization, role, title, website, vcardHasUrl, foafHomepage,
+ *   photoUrl, storageUrls, pimStorage, solidStorage, oidcIssuer, knows
+ *
+ * We extend only for properties not yet in the shared library.
+ */


Please remove all documentation comments that are actually explanatory comments.

Done. See the commit

langsamu · 2026-05-19T10:44:29Z

+ * Agent wrapping that WebID subject.
 *
- * Pure async — caching and dedup are handled by React Query (useAgent hook).
+ * Throws if the fetch or parse fails — the WebID is required for the app.


Please use JSDoc constructs like @throws where appropriate.

Done. Please see the commit.

langsamu · 2026-05-19T10:46:01Z

    queryFn: () => fetchAndParseProfile(webId!, fetchFn),
    enabled: !!webId,
    gcTime: Number.POSITIVE_INFINITY,
+    // The Agent object uses lazy getters that read from an RDF store on access.


So what? It's in-memory.

I have improved the comment to explain the actual cause. It's the infinite recursion from the library's .value getter, not the in-memory store itself. See the commit

langsamu · 2026-05-19T10:46:27Z

    gcTime: Number.POSITIVE_INFINITY,
+    // The Agent object uses lazy getters that read from an RDF store on access.
+    // React Query normally tries to compare old vs new data by walking every property,
+    // which would accidentally call those getters and cause errors. Turning this off


langsamu · 2026-05-19T10:46:50Z

+    // React Query normally tries to compare old vs new data by walking every property,
+    // which would accidentally call those getters and cause errors. Turning this off
+    // tells React Query to just use the new Agent as-is without comparing it.
+    structuralSharing: false,


Is this good practice?

yes, structuralSharing is a documented React Query option for exactly this case (class instances with getters/non-serializable data).

langsamu · 2026-05-19T10:48:22Z

+/**
+ * Safely access an agent property that may throw at runtime.
+ * Some Solid Pods store data in unexpected formats (e.g. a plain text literal
+ * where the library expects a URL/NamedNode). When that happens, @rdfjs/wrapper
+ * throws a type error. We catch it here and return null so that one bad field
+ * doesn't break the entire profile.
+ */
+function safeGet<T>(fn: () => T): T | null {
+  try {
+    return fn();
+  } catch {
+    return null;
+  }
+}
+


Well known antipattern. Please handle the exceptional case properly or model in a different way.

I updated safeGet to only catch TypeError. Please see the commit

- Revert cosmetic formatting change

langsamu

A lot has improved but there are still inconsistencies and antipatterns.
I don't want to hold you back but may I recommend following up on the remaining comments in a different change.

PreciousOritsedere · 2026-05-29T12:56:36Z

A lot has improved but there are still inconsistencies and antipatterns. I don't want to hold you back but may I recommend following up on the remaining comments in a different change.

I have pushed some commits to address all the reviews you left

PreciousOritsedere requested review from jeswr, langsamu and matthieubosquet May 19, 2026 08:37

langsamu requested changes May 19, 2026

View reviewed changes

fix: Samu's address PR review feedback on Agent.ts

b81aaf5

- Revert cosmetic formatting change

vercel Bot deployed to Preview May 22, 2026 13:13 View deployment

refactor: remove redundant comments

ed9f9c5

vercel Bot deployed to Preview May 22, 2026 13:23 View deployment

refactor: restore original Vocabulary.ts, only add hasValue

e3a90e7

vercel Bot deployed to Preview May 22, 2026 13:49 View deployment

docs: use @throws JSDoc tag in profileUtils

cc696c6

vercel Bot deployed to Preview May 22, 2026 13:54 View deployment

docs: improve structuralSharing comment in useAgent

7734a27

vercel Bot deployed to Preview May 22, 2026 14:13 View deployment

fix: narrow safeGet to only catch TypeError from malformed Pod data

070788e

vercel Bot deployed to Preview May 22, 2026 14:31 View deployment

feat: upgrade @solid/object to 0.6.0 and remove HasValue workaround

0ead6f2

vercel Bot deployed to Preview May 29, 2026 11:25 View deployment

langsamu approved these changes May 29, 2026

View reviewed changes

PreciousOritsedere merged commit dd54a86 into main May 29, 2026
2 checks passed

		return (addr != null && addr !== "") ? addr : null;
		return addr != null && addr !== "" ? addr : null;

Conversation

PreciousOritsedere commented May 19, 2026

Uh oh!

vercel Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

langsamu left a comment

Choose a reason for hiding this comment

Uh oh!

PreciousOritsedere commented May 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vercel Bot commented May 19, 2026 •

edited

Loading