New Feature: GitHub Sync #253

mdroidian · 2024-05-09T19:56:06Z

https://www.loom.com/share/0f3a0e72bc424d2f99783ab33a73f5fc

…ithub

mdroidian · 2024-05-09T19:59:42Z

src/components/GitHubSync.tsx

+                  Panel: SelectPanel,
+                  title: "Node Select",
+                  description: "Select a node to pull comments from",
+                  options: {
+                    items: [
+                      "None",
+                      ...getDiscourseNodes()
+                        .map((node) => node.text)
+                        .filter((text) => text !== "Block"),
+                    ],
+                  },
+                  defaultValue: "None",


maybe this should create be a CustomPanel and create a block on select/enter so the user can have multiple defined?

mdroidian · 2024-05-09T20:01:09Z

src/components/GitHubSync.tsx

+    return () => {
+      headerCommentObserver.forEach((o) => o.disconnect());
+      childCommentObserver.forEach((o) => o.disconnect());
+    };


adding a console.log here didn't fire. not sure why or if that means these observers aren't being cleaned up.

yea this all doens't look right, this component is not mounting at all. I would replace the

ReactDOM.render( <GitHubSyncPage commentsContainerUid={commentsContainerUid} extensionAPI={extensionAPI} />, containerDiv );

from above with just the contents of this useEffect. Actually the "observing" is happening by the time we get to renderGitHubSyncPage so I wouldn't even initiate new observers for both. I would render CommentsContainerComponent directly there, and have a separate root observer for CommentsComponent

A few problems I'm running into trying to implement this:

Observers Required
Both observers are required to re-render the buttons when a user clicks into a block.

If we just render CommentsContainerComponent and remove the observer that renders it, if a user clicks into the block the buttons will be gone until the page reloads. Maybe a necessary trade-off? But I'd like to avoid that for the UX.

RE: Root Observer
The CommentsComponent renders to children of the CommentsContainer. To know if it's a child we need the CommentsContainer el or uid which we get from a user defined query on page load. We wouldn't have that at root.

I guess we could add an attribute on block creation/page load, but we'd run into the re-render issue still (a user clicks into the block, buttons are gone until page is reloaded).

Or maybe we keep a running global cache of comment uids for the current page that clears on page unload ... we have to handle the sidebar windows too ... hmmm.

Here's a cache implementation: github-sync...github-sync-comment-uid-cache

merged into this branch: #254

dvargas92495 · 2024-05-09T22:02:58Z

This is so cool!! I wanna give this one a thoughtful review, so making a note to revisit on Saturday

dvargas92495

Nice! Lots of feedback, nothing necessarily blocking.

One approach I like doing with big meaty PRs is keep the meaty one in draft, then split out reviewable chunks, rebasing the master along the way. It's a ton more work, but this helps reviewers give more thoughtful reviews on incremental pieces and I've often found ways to make things better by forcing myself to chunk down.

dvargas92495 · 2024-05-11T10:32:00Z

docs/github-sync.md

+
+## Config Page
+
+This will be found at `[[roam/js/github-sync]]` in your graph.


First thing that stood out to me in the video - thoughts on this living within the existing node config pages instead of introducing a new page? Couple of motivations behind this:

Reducing the amount of config locations the user needs to traverse too

If we keep things defined at the node level, it will make it easier to one day port as it will be a property of a SamePage native data model (the Discourse Node, or Node) rather than something specific to Roam <> Github

Initially this felt far enough away from Discourse Graph to be on it's own. Then I ended up hooking into Discourse Nodes for their definition 😅. But I could see users want to use only this feature and none of the other dgraph features ... then it would be kind of buried / confusing.

dvargas92495 · 2024-05-11T10:34:29Z

src/components/GitHubSync.tsx

+                },
+                {
+                  // @ts-ignore
+                  Panel: TextPanel,


let's make this a SelectPanel where the options are all of the query blocks in the graph

Actually, instead of "Add some query block in your graph", I think I'd prefer an inline instance to define here. That "some query in your graph" won't be used for anything else

dvargas92495 · 2024-05-11T10:37:52Z

src/components/ExportGithub.tsx

-const initialRepos: UserRepos = [{ name: "", full_name: "" }];
+export type UserRepos = UserReposResponse["data"];
+export const initialRepos: UserRepos = [{ name: "", full_name: "" }];
+export type GitHubDestination = "Issue" | "File";


[q] Why is this an option? I'd assume we'd only want to sync with issues?

ignore, I see the use case now. To keep in sync with the array below you could instead do:

const GITHUB_DESTINATIONS = ["Issue", "File"] as const; export type GitHubDestination = (typeof GITHUB_DESTINATIONS)[number];

makes it so that adding new options is only write-once instead of write-twice

dvargas92495 · 2024-05-11T10:41:00Z

src/components/GitHubSync.tsx

+      />
+      <Button
+        icon="import"
+        title="Check for New Comments"


[nit] We should either button text this like Add Comment or have both put the title in tooltips

My intuition pushes back on this.

Adding comments is likely to happen far more often than downloading. Check for comments or even just Download places it in equal weights with Add Comment and it also felt like it took up too much space when I tried it.

Similarly, having them both just icons downplays Add Comments too much. If both are just icons, making Add Comments intent Primary might solve this, but most of the time a user is on the page they actually won't be adding comments, so it is likely too much of a distraction.

Finally, I see a future state where we query for new comments in the background and update the button to call attention to it only when there are new comments to be imported.

dvargas92495 · 2024-05-11T10:43:50Z

src/components/GitHubSync.tsx

+      .map((r: any) => {
+        const node = r[0];
+        return {
+          id: node.props["github-sync"]["comment"]["id"],


Nice! was going to suggest maintaining some metadata like this

dvargas92495 · 2024-05-11T11:01:07Z

src/components/Export.tsx

+        },
+        data: {
+          title,
+          body,


[oos] There will be formatting differences, like Roam bolding -> Github bolding.

This is where proxying through SamePage I think could be helpful, since we are defining the AtJson standard there to be interoperable across all text editors

dvargas92495 · 2024-05-11T11:02:00Z

src/components/ExportGithub.tsx

+      },
+    });
+    const installations = res.installations;
+    const APP_ID = isDev ? 882491 : 312167; // TODO - pull from process.env.GITHUB_APP_ID


[oos] yes please

dvargas92495 · 2024-05-11T11:04:54Z

src/components/Export.tsx

              gitHubAccessToken={gitHubAccessToken}
              setGitHubAccessToken={setGitHubAccessToken}
              setCanSendToGitHub={setCanSendToGitHub}
+              githubDestination={githubDestination}
+              setGithubDestination={setGithubDestination}


[nit] in general, use case specific state/props and callbacks should be defined within use case specific components. is there a way we could push all of this state within the component instead?

We could store these in localStorage and do a check onClick for these values. But if we do that for setCanSendToGitHub we would lose the state dependent UI of a disabled Export button.

dvargas92495 · 2024-05-11T11:16:19Z

src/index.ts

      renderPlayground(title, globalRefs);
    } else if (isCanvasPage(title) && !!h1.closest(".roam-article")) {
      renderTldrawCanvas(title, onloadArgs);
+    } else if (isGitHubSyncPage(title)) {


[nit] observer callbacks fire alot and roam queries within it could really slow down a graph. I would try to figure out a way to avoid doing roam queries within this method.

One way to do so is to maintain a cache of valid uids, and whenever you leave the github sync config page, that cache is updated. Then this method is only doing a lookup on the cache

I was definitely nervous about this. But unsure of a good solution.

I would expect that a user would visit the config page once then probably never again for a very long time. How would the cache be updated then?

Specifically, how would a page that is created via default Roam methods ([[, search bar, or renaming a page, for example) be added to the cache? If it's on a timer, would the UI just not show up until that timer goes off or the user triggers a refresh?

dvargas92495 · 2024-05-11T11:18:25Z

src/components/GitHubSync.tsx

+    return () => {
+      headerCommentObserver.forEach((o) => o.disconnect());
+      childCommentObserver.forEach((o) => o.disconnect());
+    };


yea this all doens't look right, this component is not mounting at all. I would replace the

ReactDOM.render( <GitHubSyncPage commentsContainerUid={commentsContainerUid} extensionAPI={extensionAPI} />, containerDiv );

from above with just the contents of this useEffect. Actually the "observing" is happening by the time we get to renderGitHubSyncPage so I wouldn't even initiate new observers for both. I would render CommentsContainerComponent directly there, and have a separate root observer for CommentsComponent

mdroidian · 2024-05-14T19:07:21Z

Sent to client for review.

Qiwei-Zhao · 2024-09-03T10:05:45Z

Thank you for developing this function. It is amazing and very useful! Will we have it in the next official release?

mdroidian · 2024-09-03T14:37:08Z

Thank you for the message @Qiwei-Zhao

This is not yet ready for release. Some bugs to work out and changes to be added.
Currently we have been focusing a lot on a separate upgrade, but we will be back to this shortly!

mdroidian · 2025-04-07T07:30:51Z

Moved to DiscourseGraphs/discourse-graph#120

mdroidian added 15 commits May 9, 2024 11:33

init

e393965

config page, roam comments

00aa560

add githubsync

a323e7a

get comments

bf89829

add comments header

f342f36

multipele: observers fixed, add comment, get comments, sent page to g…

8fc4d29

…ithub

rebase

b17b1ea

working state

79617c4

rebase

e953356

github file content encode fix

bae6519

rebase

f9f14a6

misc

402ada7

use existing onClose()

1b2bd93

use createConfigObserver instead of renderConfigPage

dab592a

misc

32452f3

mdroidian requested a review from dvargas92495 May 9, 2024 19:56

mdroidian commented May 9, 2024

View reviewed changes

mdroidian added 5 commits May 10, 2024 09:21

save repo to issue and pull from props when downloading comments

af67257

better copy

91f3aba

add github api reference links

d8b5e26

add docs

8565186

link to docs from Readme

c79a736

dvargas92495 approved these changes May 11, 2024

View reviewed changes

mdroidian added 5 commits May 14, 2024 09:01

add comment cache (#254)

d3d7c04

better error messages

82c8df9

change Comments Block from reference to inline query

6b6b52f

misc PR Feedback

f899fee

check for comment before sending apiPost

6a356e5

mdroidian added 5 commits May 14, 2024 12:35

re-add NODETEXT and NODEUID for sidebar support

6db4cbf

handle sidebar pages

a2ffa74

move props to sub component, store in localStorage (#255)

8218df8

stop access-token checks if we've alread received from auth window

57b2b2f

update docs

d85d3f6

remove isVisible, replace with JSX conditional

72ed156

mdroidian marked this pull request as draft October 12, 2024 13:10

mdroidian closed this Apr 7, 2025


		## Config Page

		This will be found at `[[roam/js/github-sync]]` in your graph.

New Feature: GitHub Sync #253

New Feature: GitHub Sync #253

Uh oh!

Conversation

mdroidian commented May 9, 2024

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!

dvargas92495 commented May 9, 2024

Uh oh!

dvargas92495 left a comment

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!

mdroidian commented May 14, 2024

Uh oh!

Qiwei-Zhao commented Sep 3, 2024

Uh oh!

mdroidian commented Sep 3, 2024

Uh oh!

mdroidian commented Apr 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants