-
Notifications
You must be signed in to change notification settings - Fork 37
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
✨ Pass queue config to the server query when fetching statuses #263
Conversation
@@ -65,6 +65,9 @@ export const LocalPreferences = () => { | |||
<p className="text-sm mb-2"> | |||
You can choose to make media content (video and image) with the | |||
following labels appear on your screen with your preferred filter. | |||
<br /> |
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.
Not related to the PR but adding additional clarification as requested by mods.
@@ -46,7 +46,9 @@ export const ExternalLabelerConfig = () => { | |||
subscriptions there. | |||
</p> | |||
<p className="mt-1 text-sm text-gray-900 dark:text-gray-200"> | |||
You can unsubscribe from any external labeler at any time. | |||
You can unsubscribe from any external labeler at any time. This is |
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.
Not related to the PR but adding additional clarification as requested by mods.
@@ -41,8 +41,8 @@ export function LabelerConfig() { | |||
)} | |||
|
|||
<ServerConfig /> | |||
<LocalPreferences /> |
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.
Not related to the PR but relocating the panels as requested by mods.
}) | ||
} | ||
|
||
const TextWithLinks: React.FC<{ text: string }> = ({ text }) => { |
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.
Not related to the PR but mods reported that sometimes they leave links to verification/investigation content and the links always cause the layout to break. This is basically looking for links in comment and making sure they are used wrapped in a tag and wraps the text properly to next line.
const parts = text.split(urlRegex) | ||
|
||
return parts.map((part, index) => { | ||
if (urlRegex.test(part)) { |
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.
Global (/g
) regexes are stateful when used with .test()
, taking into account the last matched index. I think that this could lead to missed matches here.
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.
For example:
const regex = /a/g
regex.test('a') // true
regex.test('a') // false
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.
again, not sure how accurate I am with this but it seems to be only an issue when applied to same string and since we're applying it on different parts
I think this works fine...? I did test this with various possible inputs where it may fail but it seems to work fine but happy to make changes if we find issues down the road.
const parts = text.split(urlRegex) | ||
|
||
return parts.map((part, index) => { |
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.
The parts matching the regex wont be included in parts
— I think this will effectively filter out links.
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.
my regex-foo is not good at all but stackoverflow said wrapping it in parentheses makes it a capture group which is why the matches will be included in the returned array.
Depends on bluesky-social/atproto#3280
This PR removes the client side queue splitting and passes the queue config to the request when calling
queryStatuses
so that the db query on the server can take care of the splitting.