From 186e99e23d73a2e80f966ba8195ea7a305f0ed0d Mon Sep 17 00:00:00 2001 From: Peter Gaultney Date: Mon, 17 Aug 2026 14:47:00 -0400 Subject: [PATCH] fix: let the identity directory supply appearance a provider lacks `resolveAuthorIdentity` takes the first resolver that returns anything and discards the rest, so a provider record with a name but no picture beats a configured entry that has both. The `identities` setting then appears to do nothing for anyone the provider recognises - which, with Relay signed in to a directory of the whole team, is everyone. Who an ID belongs to and what they look like come from different places. A provider knows the first; whether it knows the second depends on the account it signed in with. Relay carries a picture only when the OAuth provider supplied one - Microsoft supplies none - and a colour only for the local user, drawn at random per session for its own presence cursors. So every comment author renders as initials in one uniform accent colour. A provider now wins on identity while the directory fills in picture, colour, and colourLight field by field. The directory never overrides something the provider does carry, and an entry that sets only one field does not blank the others. --- src/identity/providers.ts | 27 ++++++++++++ src/main.ts | 48 ++++++++++++++++++++- tests/unit/identity/providers.test.ts | 61 +++++++++++++++++++++++++++ 3 files changed, 134 insertions(+), 2 deletions(-) diff --git a/src/identity/providers.ts b/src/identity/providers.ts index 4976fc5..3c88baf 100644 --- a/src/identity/providers.ts +++ b/src/identity/providers.ts @@ -68,6 +68,33 @@ export class ConfiguredIdentityResolver implements IdentityResolver { } } +/** + * Fill a resolved identity's missing appearance from the configured directory. + * + * Who an ID belongs to and what they look like come from different places. A live provider knows + * the first; whether it knows the second depends on the account it signed in with - Relay carries + * a picture only when the OAuth provider supplied one, and a colour only for the local user. The + * configured directory is where someone records what a provider cannot tell them. + * + * So a provider wins on identity while the directory fills the gaps, rather than the first + * resolver with any answer at all taking the whole record. Otherwise directory entries are ignored + * for exactly the people a provider already recognises, which is normally all of them - and the + * setting appears to do nothing. + */ +export function withConfiguredDecoration( + identity: Identity, + configured: Identity | null, +): Identity { + if (!configured) return identity; + + return { + ...identity, + picture: identity.picture ?? configured.picture, + color: identity.color ?? configured.color, + colorLight: identity.colorLight ?? configured.colorLight, + }; +} + export type RelayIdentitySupportStatus = | "not-installed" | "unsupported" diff --git a/src/main.ts b/src/main.ts index 83c817d..d235432 100644 --- a/src/main.ts +++ b/src/main.ts @@ -72,6 +72,7 @@ import { getRelayIdentitySupportStatus, providerById, selectIdentityProvider, + withConfiguredDecoration, } from "./identity/providers"; import type { Identity, @@ -1353,7 +1354,8 @@ export default class RelayCommentsPlugin if (!resolver.isAvailable()) continue; try { const identity = await resolver.resolveUser(author, path); - if (identity) return providerIdentity(identity, resolver.id); + if (identity) + return this.decorated(identity, resolver, author, path); } catch { // Resolver failures degrade to the unresolved author value // instead of breaking review rendering. @@ -1364,6 +1366,47 @@ export default class RelayCommentsPlugin return null; } + /** + * One resolver's answer, with appearance the directory carries and it does not. + * + * Skipped when the directory is itself the answering resolver, which has nothing to fill in + * from. + */ + private decorated( + identity: Identity, + resolver: IdentityResolver, + author: string, + path: string, + ): ReviewerIdentity { + if (resolver.id === this.configuredIdentityResolver.id) + return providerIdentity(identity, resolver.id); + + return providerIdentity( + withConfiguredDecoration( + identity, + this.resolveConfiguredIdentity(author, path), + ), + resolver.id, + ); + } + + /** The configured directory's record for an author, for decoration only. */ + private resolveConfiguredIdentity( + author: string, + path: string, + ): Identity | null { + if (!this.configuredIdentityResolver.isAvailable()) return null; + + try { + return ( + this.configuredIdentityResolver.resolveUserSnapshot?.(author, path) ?? + null + ); + } catch { + return null; + } + } + private resolveAuthorIdentitySnapshot( author: string, path: string, @@ -1378,7 +1421,8 @@ export default class RelayCommentsPlugin if (!resolver.resolveUserSnapshot) return undefined; try { const identity = resolver.resolveUserSnapshot(author, path); - if (identity) return providerIdentity(identity, resolver.id); + if (identity) + return this.decorated(identity, resolver, author, path); } catch { return undefined; } diff --git a/tests/unit/identity/providers.test.ts b/tests/unit/identity/providers.test.ts index c3bd5b5..85b6f96 100644 --- a/tests/unit/identity/providers.test.ts +++ b/tests/unit/identity/providers.test.ts @@ -7,8 +7,69 @@ import { ObsidianSyncIdentityProvider, RelayIdentityProvider, selectIdentityProvider, + withConfiguredDecoration, } from "src/identity/providers"; +describe("configured decoration", () => { + it("supplies the appearance a provider does not carry", () => { + expect( + withConfiguredDecoration( + { id: "u1", name: "Sarah Rilling" }, + { + id: "u1", + name: "whatever the directory calls them", + picture: "https://example.com/sarah.jpg", + color: "#30bced", + colorLight: "#30bced33", + }, + ), + ).toEqual({ + id: "u1", + // The provider stays authoritative about who this is. + name: "Sarah Rilling", + picture: "https://example.com/sarah.jpg", + color: "#30bced", + colorLight: "#30bced33", + }); + }); + + it("never overrides appearance a provider does carry", () => { + expect( + withConfiguredDecoration( + { id: "u1", name: "Sarah", picture: "provider.jpg", color: "#111111" }, + { + id: "u1", + name: "Sarah", + picture: "configured.jpg", + color: "#222222", + }, + ), + ).toMatchObject({ picture: "provider.jpg", color: "#111111" }); + }); + + it("leaves an identity alone when the directory has no entry", () => { + const identity = { id: "u1", name: "Sarah" }; + + expect(withConfiguredDecoration(identity, null)).toEqual(identity); + }); + + it("fills each field independently", () => { + // A directory entry that only sets a colour must not blank out a picture, and vice versa. + expect( + withConfiguredDecoration( + { id: "u1", name: "Sarah", picture: "provider.jpg" }, + { id: "u1", name: "Sarah", color: "#30bced" }, + ), + ).toEqual({ + id: "u1", + name: "Sarah", + picture: "provider.jpg", + color: "#30bced", + colorLight: undefined, + }); + }); +}); + describe("configured identity directory", () => { it("resolves other authors without acting as the current user", async () => { const provider = new ConfiguredIdentityResolver(() => [