-
Notifications
You must be signed in to change notification settings - Fork 25
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
OAuth integration #128
OAuth integration #128
Conversation
Your Render PR Server URL is https://ozone-staging-pr-128.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-cpd183e74orc73bh2210. |
Your Render PR Server URL is https://ozone-sandbox-pr-128.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-cpd183u74orc73bh2270. |
461371b
to
9323aac
Compare
ad13d2e
to
ca825f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving some comments after reviewing nearly 60% of the changes. Will do another pass once you get the rebased changes in.
Again, apologies for the huge conflict you're handling right now and LMK if I can help!
ca825f2
to
76026e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow this is a massive piece of work! thank you for powering through the conflict resolution and it looks quite good. left some comments and questions but nothing major popped out.
would love to get an extra pair of eyes from @devinivy and then do some testing with our mod team before creating a distro with this so let's hold on to merging until we get a distro out this week. will come back and approve once that's done.
927b2d2
to
b24adb2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
🐛 Add explicit resolution to avoid cypress install error 🐛 Fix yarn.lock issue ✅ Fix tests ✨ Replace deprecated BskyAgent 🚧 Fixing e2e suite ✅ Specify handle resolver url
Co-authored-by: devin ivy <[email protected]>
@@ -9,10 +9,16 @@ export async function getConfig(): Promise<OzoneConfig> { | |||
if (labelerDid) { | |||
doc = await resolveDidDocData(labelerDid) | |||
const labelerUrl = doc && getServiceUrlFromDoc(doc, 'atproto_labeler') | |||
if (labelerUrl) { | |||
meta = await getOzoneMeta(labelerUrl) | |||
if (process.env.NODE_ENV === 'development' && doc && labelerUrl) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't existence of labelerUrl imply that doc exists? not clear why we need to check for && doc
here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is solely for the purpose of making this work with yarn dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with dev-env
from atproto, there is no .well-known
file, so the getOzoneMeta
results in a 404.
I'm open to any other solution for this :-)
export const PLC_DIRECTORY_URL = | ||
process.env.NEXT_PUBLIC_PLC_DIRECTORY_URL || `https://plc.directory` | ||
process.env.NEXT_PUBLIC_PLC_DIRECTORY_URL || | ||
(process.env.NODE_ENV === 'development' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if hardcoding these based on NODE_ENV is very useful. next.js easily picks up local .env files and that also makes it quite easy to easily change these locally without having to change code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to add more context, it makes total sense that when developing locally, we'd want to point to local atproto dev env but often, I point at staging or production to check UI things which is adds extra step when committing changes etc. so, this is more of a devex consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this so that it works with yarn dev
without any additional .env
file whatsoever. I am happy to remove this if that breaks your flow.
* 🐛 Allow Only/Exclude/Indifferent filters for appeal status * 🐛 Fix page title * Line up logic with surrounding code Co-authored-by: devin ivy <[email protected]> --------- Co-authored-by: devin ivy <[email protected]>
Running locally:
pnpm i && pnpm build && pnpm dev
fromatproto
's feat-oauth-client branchNEXT_PUBLIC_PLC_DIRECTORY_URL="http://localhost:2582" NEXT_PUBLIC_SOCIAL_APP_URL="http://localhost:2584" NEXT_PUBLIC_OZONE_SERVICE_DID="<ENTER_COPIED_OZONE_DID_HERE>" y dev
http://127.0.0.1:3000
(make sure to use127.0.0.1
and notlocalhost
). Alternatively, start a reverse proxy using:ngrok http 3000
and visit the proxy's URL instead