-
Notifications
You must be signed in to change notification settings - Fork 10
Adds Vapor backend to project #109
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
base: develop
Are you sure you want to change the base?
Conversation
|
Claude finished @loloop's task —— View job PR Review: Vapor Backend Addition
|
|
Claude finished @loloop's task —— View job PR Review: Vapor Backend Addition
|
|
Claude finished @loloop's task —— View job PR Review: Vapor Backend AdditionReviewing this PR that adds a Vapor backend and creates a shared CocoaHeadsCore package. Review Checklist:
Review SummaryThis is a well-structured PR that introduces a Vapor backend to move functionality away from CloudKit dependencies. The architecture changes are thoughtful and the implementation is generally solid. ✅ StrengthsArchitecture & Design:
Backend Implementation:
Infrastructure:
|
unnamedd
left a comment
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.
@loloop, thanks for the effort here. As I am not working—not even a bit—in the CH projects, I do not see myself in a position to block your work with a "Request changes" from GitHub's Pull Request; however, I’m raising some points here for your appreciation.
Give a check, and if you think they are addressable, apply the comments and, if you want me to give you a second review, just re-request a review and I will do so with great pleasure.
| guard | ||
| let url = URL(string: meetup.url), | ||
| case (let data, _) = try await URLSession.shared.data(from: url), | ||
| let html = String(data: data, encoding: .utf8) | ||
| else { | ||
| throw Error.urlError | ||
| } |
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 suggest you to split into two different guards here. You are not being clear enough when raising errors.
If something goes wrong with the URL, then you will throw Error.urlError, however, in case you get an error while you are requesting the content from that URL using URLSession or get an error while you are converting from the received data into a String, you will also throw Error.urlError. You had to separate at least in two more guards to throw the proper error.
| throw Error.urlError | ||
| } | ||
|
|
||
| guard !url.absoluteString.contains("Entrar") else { |
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.
is "Entrar" a safe string to use?
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.
Used to be, but meetup has released a completely new design while I was working on moving this code around, so I'll need to do more testing to figure out if everything here still works
| case .addressError: | ||
| "Erro ao buscar o endereço" | ||
| case .coordinateError: | ||
| "Erro no parse das coordenadas" |
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.
If we are going to use the error messages in portuguese, perhaps here we should use "extrair" instead of "parse". What do you think?
| "Erro no parse das coordenadas" | |
| "Erro ao extrair as coordenadas" |
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.
These errors used to be part of the UI when SwiftSoup was directly integrated in the app. I'm totally down to translate these messages to english now that they're part of the API
| case .coordinateError: | ||
| "Erro no parse das coordenadas" | ||
| case .dateError: | ||
| "Não consegui encontrar ou fazer decode da data" |
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.
| "Não consegui encontrar ou fazer decode da data" | |
| "Não foi possível encontrar ou decodificar a data" |
|
|
||
| extension String { | ||
| fileprivate func removingFirstOccurrence(of word: String) -> String { | ||
| guard let range = range(of: "\\b\(word)\\b", options: [.regularExpression, .caseInsensitive]) else { |
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.
Here, we have just one use case, but I was wondering if would be a good place to make use of RegexBuilder here
| // Created by Mauricio on 5/14/25. | ||
| // | ||
|
|
||
| import CocoaHeadsCore |
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.
CocoaHeadsKit or CocoaHeadsCore? I see them being mixed 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.
CocoaHeadsKit is the package that provides all the code for the apps, CocoaHeadsCore is a new package that provides shared code between the apps and the Vapor backend
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.
perhaps it would be better if they were renamed in order to avoid confusion. To me, as you probably noticed, they were just getting mixed with each other, while in fact, they do separate things, but I am completely out of ideas to suggest something better haha
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 think better documenting them would help with this
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.
indeed, it would
|
Claude finished @loloop's task —— View job PR Review: Vapor Backend Addition
|
|
Claude finished @loloop's task —— View job PR Review: Vapor Backend AdditionReviewing this PR that adds a Vapor backend and creates a shared CocoaHeadsCore package. Review Checklist:
Review SummaryThis is a significant architectural improvement that successfully moves from SwiftSoup dependency to a modern Vapor backend with Firecrawl integration. The code quality is generally good, but there are critical issues that must be addressed before merging. ✅ Major Improvements Since Previous Reviews1. Modernized Event Scraping:
2. Architecture & Design:
3. Infrastructure:
🚨 Critical Issues1. Invalid macOS Platform Versions ( .macOS(.v26) // This version doesn't exist - will cause build failures
2. URL Security Vulnerability ( guard let url = URL(string: meetup.url) else {
throw MeetupError.urlError
}
3. Missing API Key Validation:
|

This PR intends to:
• Add a vapor backend starter base to the project
• Remove the SwiftSoup dependency for meetup fetching from the app, delegating that feature to the backend instead
Tasks:
Next steps