Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

channels: custom extension, configuration and post types #4109

Draft
wants to merge 11 commits into
base: release-channels-updates
Choose a base branch
from

Conversation

arthyn
Copy link
Member

@arthyn arthyn commented Oct 23, 2024

PR Checklist

  • Includes changes to desk files
  • Describes how you tested the PR locally (test ship vs livenet)
  • If a new feature, includes automated tests
  • Comments added anywhere logic may be confusing without context

@arthyn arthyn changed the base branch from develop to mp/channels October 23, 2024 19:09
interface MessageInput {
type: string;
postType: string;
configuration: Record<string, string>;
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you see in configuration? my guess is like – for a button input, what does the button "say" (in this specific channel). if so, I vote we skip this for now – we can do a lot without it, and I think I want to push to make these hoon snippets as well. (unless these string values are actually snippets, hm?)

Copy link
Member Author

Choose a reason for hiding this comment

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

right yeah, what would the hoon snippets do here?

Copy link
Contributor

Choose a reason for hiding this comment

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

example: button input, snippet calls out to an LLM API, which gives back a fortune cookie response, which is used as the post body

Copy link
Contributor

Choose a reason for hiding this comment

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

another one: audio message, hoon applies a user-defined audio effect over the wavetable

Copy link
Contributor

Choose a reason for hiding this comment

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

(tbc, i think this means client-side hoon execution to have a decent UX...)

Copy link
Contributor

Choose a reason for hiding this comment

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

ftr, I changed my mind on this – I still think that Hoon snippets would be great here, but user configuration through parameters seems pretty important.

(Is there any chance that JSON values look enough like Hoon expressions? like, if we accept values like 3 and 'blue', could we change in the future to say "those were all actually simple Hoon expressions that didn't have any operators")

version: 1;
messageInputs: MessageInput[];
defaultContentRenderers?: ContentRenderer[];
defaultPostCollectionRenderer?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is my fault, but I think we should probably drop the default from defaultPostCollectionRenderer, as I don't see a way to have multiple collection renderers – do you? (the only thing that comes to mind is for nested timelines like replies, reactions, etc)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I don't think we'll have multiple


interface ChannelMetadataSchemaV1 {
version: 1;
messageInputs: MessageInput[];
Copy link
Contributor

Choose a reason for hiding this comment

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

thinking out loud: considering array vs dictionary (mapping ID -> MessageInput) here – array is nice because we'll probably need to order these inputs in the UI somehow, and it'd be good to give control of that to the configuring user.

if we did a dictionary keyed on postType, we could enforce no duplicate postType values

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion here because I don't know what the "multiple inputs" UX is going to be. I guess the no-duplicates enforcement isn't that important because we can just do a runtime check.

interface ChannelMetadataSchemaV1 {
version: 1;
messageInputs: MessageInput[];
defaultContentRenderers?: ContentRenderer[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be better as a map of post-filter -> renderer; i.e. change ContentRenderer to simply { using: string } (or { rendererId: string } – sorry for waffling), and change this to defaultContentRenderers?: { [match: string]: ContentRenderer }. this enforces no duplicate post-filters, and makes it easier for someone to walk through the logic of "which renderer will this use" in their head.

Copy link
Contributor

Choose a reason for hiding this comment

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

not a strong opinion though

rendererOverride?: string;
}

interface MessageInput {
Copy link
Contributor

Choose a reason for hiding this comment

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

my fault – can we change this to PostInput to be consistent? (also at ChannelMetadataSchemaV1#messageInputs)

Copy link
Member Author

Choose a reason for hiding this comment

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

yep

version: number;
name: string;
src: string;
compiled: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking we might need some indicator of whether this source code actually compiles or not so we could surface an error that something is wrong with your code.

Copy link
Contributor

Choose a reason for hiding this comment

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

seems useful - i read this as "is this code precompiled" - maybe "valid" would be more intuitive?


interface Hook {
version: number;
name: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it sounds like name needs to be unique – as a non-urbit user, name feels like "display name" instead of a machine id. id or key is more communicative of the required uniqueness

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah this is good feedback will change

type ChannelMetadataSchema = ChannelMetadataSchemaV1;

interface Hook {
version: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why use a version number instead of a created timestamp? seems like potentially one less thing to keep track of

Copy link
Member Author

Choose a reason for hiding this comment

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

I would think that you'd want to know which iteration of this particular hook is running

}

interface Hooks {
allowed: Hook[];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: validate matches the "first person present verb" form better, and avoids ambiguity of "these are the hooks that are allowed"

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed

export type KindData = KindDataDiary | KindDataChat | KindDataHeap;
export type Kind = 'heap' | 'diary' | 'chat';
export type KindDataCustom = {
custom: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: in an extreme of the custom channels world, everything will end up with kind custom – aesthetically, I wonder if there's another term that we could use to support that. ("user"?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think at that point we'd just change our API so that kind-data goes away and we just have a post-type field or w/e.

Base automatically changed from mp/channels to release-channels-updates November 8, 2024 16:27
@arthyn arthyn changed the base branch from release-channels-updates to develop November 8, 2024 17:06
@arthyn arthyn changed the base branch from develop to release-channels-updates November 8, 2024 17:06
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.

2 participants