Reactive pos-label and pos-description by jg10-mastodon-social · Pull Request #197 · pod-os/PodOS

jg10-mastodon-social · 2026-03-01T05:59:09Z

Closes #175

jg10-mastodon-social · 2026-03-01T06:03:43Z

core/src/thing/Thing.ts

+      ),
+      startWith(this.anyValue(...predicateUris)),
+    );
+  }


This is a bit different to other observe functions because I've tried to use the stream of changes directly and avoid unnecessary calls to anyValue.

I should perhaps have implemented this through multiple commits to improve clarity, but I did attempt to follow TDD so the tests should correspond to features of this function.

question: Why is it important that anyValue is not called? Is it that expensive?

I've had performance issues in my experiments generally but I don't yet know whether anyValue is a problem specifically.

rdflib.js hopefully uses indexes, but whatever it does internally to search through the store to lookup the subject and predicate would be much more expensive than just reacting to the streamed value.

So this might be premature optimisation and we could call anyValue here too - though it would also be useful to give a precedent of what can be done with the rxjs architecture. If rxjs ends up being too confusing maybe we should be looking at other reactive architectures for the future.

jg10-mastodon-social · 2026-03-01T06:42:12Z

core/src/thing/Thing.label.spec.ts

+      const observable = thing.observeLabel();
+      observable.subscribe(subscriber);
+
+      expect(subscriber.mock.lastCall).toEqual([thing.label()]);


While observeLabel doesn't call label at all, it does call labelFromUri so I thought this might be a sufficient test without duplicating all of "if nothing is found in the store".

From a TDD perspective, I wrote this as a failing test before implementing

.pipe(map((value) => value ?? labelFromUri(this.uri)));

I think this is reasonable

…bscribe

jg10-mastodon-social · 2026-03-21T07:11:49Z

I'm currently planning to implement the e2e test only when the dynamic pane feature is live.

This update can be manually explored by adding a label or description literal in a generic thing page when there isn't one yet. The label and description in the overview then update live.
This use case could be implemented already now as an e2e test, but it's a bit contrived, so I would prefer not to.

angelo-v

I am not fully through with the review, but I left some comments you can alreay work on and perhaps answer some of my questions. I try to come back to it asap (maybe monday)

Overall this is good work and a valuable addition, even if some rework is still needed

core/CHANGELOG.md

angelo-v · 2026-04-10T09:30:08Z

core/src/thing/Thing.ts

+   */
+  observeLabel() {
+    return this.observeAnyValue(
+      "http://www.w3.org/2006/vcard/ns#fn",


suggestion: do not duplicate the label predicates, but extract them to a constant that is used both by label() and observeLabel

angelo-v · 2026-04-10T09:35:53Z

core/src/thing/Thing.ts

+
+    const currentValue = this.anyValue(...predicateUris);
+    const dirty$ =
+      typeof currentValue === "undefined"


question: Why not just currentValue === undefined?

That is indeed simpler! I somehow learnt to use typeof 😅

angelo-v · 2026-04-10T09:38:51Z

core/src/thing/Thing.ts

+   */
+  observeDescription() {
+    return this.observeAnyValue(
+      "http://purl.org/dc/terms/description",


suggestion: same as for label

angelo-v · 2026-04-10T09:46:48Z

elements/src/components/pos-description/pos-description.tsx


  receiveResource = (resource: Thing) => {
-    this.resource = resource;
+    resource


issue: I think we should cancel the old subscription when receiving a new resource. You did so in pos-switch. Is there a reason you are not doing it here, or did you just forget?

I think I was just working on them in parallel and hadn't yet thought to apply that pattern here.

angelo-v · 2026-04-10T10:12:47Z

core/src/thing/Thing.label.spec.ts

+      const observable = thing.observeLabel();
+      observable.subscribe(subscriber);
+
+      expect(subscriber.mock.lastCall).toEqual([thing.label()]);


I think this is reasonable

angelo-v · 2026-04-10T10:26:07Z

core/src/thing/Thing.anyValue.spec.ts

+      expect(subscriber).toHaveBeenCalledTimes(2);
+    });
+
+    it("pushes a value if none was present, without calling anyValue again", () => {


suggestion: This test does not use the setup from before each. Split the tests that need the setup and those who dont into separate nested describe blocks

angelo-v · 2026-04-10T10:26:44Z

core/src/thing/Thing.ts

+      ),
+      startWith(this.anyValue(...predicateUris)),
+    );
+  }


question: Why is it important that anyValue is not called? Is it that expensive?

angelo-v · 2026-04-10T10:35:15Z

core/src/thing/Thing.ts

+    return merge(dirty$, addition$).pipe(
+      debounceTime(250),
+      map((value) =>
+        typeof value === "string" ? value : this.anyValue(...predicateUris),


typeof value === "string"

issue: this check really confused me. It is basically a dirty way to check whether the observed value comes from addtions

thought: I am still having trouble to fully understand the function and the role of the dirty$ observable. Perhaps you can explain a bit. I need to finish my review for now, but will come back to this. If you find the time to explain a bit in the meantime this is much appreciated. My gut feeling is that the whole function could and should be simplified.

Yes, that's a switch between calling anyValue and using the value from additions

I'm still getting my head around rxjs and its functionality but the intention is that addition is only read when the current value of anyValue is dirty, i.e. it hasn't been set yet or there has been a removal.

We could also simplify and just trigger for every new addition (there's already a debounce to handle concurrent removals). This would cause the label to change, i.e. a different anyValue would be selected just because there's more than one defined.
The current behaviour is intended to guarantee that once anyValue has been found, additional values in the data will be ignored.

angelo-v · 2026-04-10T10:42:48Z

core/src/thing/Thing.anyValue.spec.ts

    });
  });
+
+  describe("observeAnyValue", () => {


thought: I think there are not enough tests given the complexity of the function. What about having multiple different (matching) predicates that "come and go" over time. How does the value change or stay consistent?

I did try to cover a lot of behaviours but I admit I didn't try to lay out all possible combinations of possible input states.

The existing tests specifically describe the behaviour I was trying to achieve, which could perhaps be simplified at the expense of some user surprise and/or marginal but unknown performance impact.

… cycle

Co-authored-by: Angelo Veltens <angelo+github.com@veltens.org>

angelo-v · 2026-04-10T11:07:47Z

core/src/thing/Thing.anyValue.spec.ts

    });
+
+    // TODO make this green
+    it.skip("emits new value after add-remove-add cycle", () => {


@jg10-mastodon-social this actually highlights a bug and supports my gut feeling that test coverage is not high enough

jg10-mastodon-social commented Mar 1, 2026

View reviewed changes

jg10-mastodon-social mentioned this pull request Mar 1, 2026

Reactivity to state changes #37

Open

jg10-mastodon-social added 10 commits March 21, 2026 11:09

feat(Thing): add Thing.observeAnyValue

2419e69

feat(Thing): add observeLabel

b32b89e

test(Thing): observeLabel checks all listed predicates

e54afe9

test(Thing): observeLabel follows observeAnyValue behaviour

dff24d4

feat(Thing): add observeDescription

e34b0a3

feat(elements): pos-label uses Thing.observeLabel

ab0d09f

test(pos-label): updates label when value changes

3e0f4b8

feat(elements): pos-description uses observeDescription

0d68a70

test(elements): migrate pos-list.integration to observeLabel mock

6f4adac

test(elements): migrate pos-resource.integration to observeLabel mock

e85a6f2

jg10-mastodon-social force-pushed the feat/reactive-label-description branch from 25ffd0b to e85a6f2 Compare March 21, 2026 00:10

jg10-mastodon-social added 10 commits March 21, 2026 11:15

test(elements): migrate pos-rich-link.integration to observe mocks

5bb991b

test(elements): migrate pos-switch.integration to observeLabel mock

5598ebb

test(elements): add observeLabel to mockPodOs

e0bb223

feat(pos-value): updates when data changes using observeAnyValue

3f4e846

feat(pos-value): updates on predicate change

13ecf50

doc(elements): rebuild readme

8519ecd

doc(elements): add reactive pos-value, label, description to changelog

4805c6b

doc(core): add observe anyValue, label, description to changelog

eca7c89

chore(elements): disconnected subject should complete instead of unsu…

fec9f53

…bscribe

fix(core): Thing.observeAnyValue pushes a value if none was present

4cb507e

jg10-mastodon-social requested a review from angelo-v March 21, 2026 07:12

jg10-mastodon-social marked this pull request as ready for review March 21, 2026 07:12

angelo-v requested changes Apr 10, 2026

View reviewed changes

angelo-v and others added 2 commits April 10, 2026 13:06

test(observeAnyValue): add test that highlights bug in add-remove-add…

1d8eacf

… cycle

Update core/CHANGELOG.md

ea41051

Co-authored-by: Angelo Veltens <angelo+github.com@veltens.org>

angelo-v self-requested a review April 13, 2026 18:29

angelo-v reviewed Apr 13, 2026

View reviewed changes

Conversation

jg10-mastodon-social commented Mar 1, 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!

jg10-mastodon-social commented Mar 21, 2026

Uh oh!

angelo-v left a comment

Choose a reason for hiding this comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jg10-mastodon-social commented Mar 1, 2026 •

edited

Loading