WIP: git sync #375
WIP: git sync #375
Conversation
deps.edn has a local reference for the .jar file for mockingbird, so you'll need to build the jar and reference your own local path.
WalkthroughThis pull request introduces several significant changes across the Moclojer project, focusing on enhancing authentication, webhook handling, and preparing infrastructure for Git repository synchronization. The modifications span multiple components, including dependencies, configuration files, frontend components, and backend API interceptors. The changes lay the groundwork for implementing Git repository synchronization for paid plans, with a particular emphasis on GitHub integration. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/front/app/dashboard/events.cljs (1)
268-279:⚠️ Potential issueMultiple issues in username update handler.
- Incorrect event registration (same as previous review).
- Debug logging in production code.
- Missing error handling.
- Incorrect URL construction.
Apply this diff to fix the issues:
-(refx/reg-event-db +(refx/reg-event-fx :app.dashboard/set-new-username - (fn [{{:keys [current-user]} :db} [_ username-to-save]] - (js/console.log username-to-save current-user) - (let [access-token (:access-token current-user) - user-id (:user_id (:user current-user))] + (fn [{db :db} [_ username-to-save]] + (let [access-token (get-in db [:current-user :access-token]) + user-id (get-in db [:current-user :user :user_id])] {:http {:url (str "user/" user-id) :method :post :headers {"authorization" (str "Bearer " access-token)} :body {:username username-to-save} - :on-success [] - :on-failure []}}))) + :on-success [:app.dashboard/set-new-username-success] + :on-failure [:app.dashboard/set-new-username-failure]}})))Additional handlers needed:
(refx/reg-event-fx :app.dashboard/set-new-username-success (fn [{db :db} _] {:notification {:type :info :content "Username updated successfully!"} :dispatch [:app.dashboard/toggle-settings]})) (refx/reg-event-fx :app.dashboard/set-new-username-failure (fn [{db :db} _] {:notification {:type :error :content "Failed to update username."}}))src/front/app/dashboard/editor.cljs (1)
96-105: 🛠️ Refactor suggestionFix push button logic and improve UX.
The button implementation needs improvements for better user experience and error handling.
Apply this diff to enhance the implementation:
(defnc push-button [{:keys [mock]}] - (let [mock-valid? (refx/use-sub [:app.dashboard/mock-valid?])] + (let [mock-valid? (refx/use-sub [:app.dashboard/mock-valid?]) + loading? (refx/use-sub [:app.dashboard/loading-push-mock]) + error? (refx/use-sub [:app.dashboard/push-error])] (d/button {:class-name (str "px-3 py-2 bg-gray-500 rounded-lg flex justify-center items-center btn-add space-x-2 " - (when-not mock-valid? " opacity-50 cursor-not-allowed")) - :on-click #(when mock-valid? + (when (or (not mock-valid?) loading?) " opacity-50 cursor-not-allowed")) + :disabled (or (not mock-valid?) loading?) + :on-click #(when (and mock-valid? (not loading?)) (refx/dispatch-sync [:app.dashboard/push-mock mock]) (refx/dispatch-sync [:app.dashboard/reset-mock-pub-stts (:id mock)]))} - (d/div {:class-name "text-white text-xs font-bold leading-[18px]"} " push to origin") - ($ svg/save)))) + (d/div {:class-name (str "text-white text-xs font-bold leading-[18px]" + (when error? " text-red-200"))} + (cond + loading? "pushing..." + error? "push failed" + :else "push to origin")) + (cond + loading? ($ svg/spinner) + error? ($ svg/error) + :else ($ svg/save)))))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/back/api/controllers/user.clj(1 hunks)src/dev/shadow/hooks.clj(1 hunks)src/front/app/dashboard/editor.cljs(1 hunks)src/front/app/dashboard/events.cljs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: WIP
🔇 Additional comments (7)
src/back/api/controllers/user.clj (4)
41-46: Use kebab-case for variable and keyword names.
Usinggit-usernameinstead ofgit_usernamebetter aligns with Clojure conventions.
48-57: Looks good.
The function's error handling and logging are consistent with typical patterns in the codebase.
59-68: Consider adding valid email format checks for early failure.
This prevents invalid email strings from reaching the database and failing later.
70-81: Enrich the logging context with theinstall-id.
Including all key fields in the logging context facilitates easier debugging and traceability.src/front/app/dashboard/events.cljs (3)
24-25: LGTM! Clean formatting changes.The formatting changes improve readability while preserving the existing logic.
27-33: LGTM! Well-structured settings toggle handler.The implementation follows the established pattern and correctly handles the db state toggle.
281-297: Clean up debug statements and improve code structure.The handler contains unnecessary debug statements and can be simplified.
Apply this diff to clean up the code:
(refx/reg-event-fx :app.dashboard/push-mock (fn [{db :db} [_ mock]] - (let [current-user (:current-user db)] - (prn db) - (prn mock) - ;; mock is already at source its sha is not nil + (let [access-token (get-in db [:current-user :access-token])] {:http {:url "/mocks" :method :put - :headers {"authorization" (str "Bearer " (:access-token (-> db :current-user)))} + :headers {"authorization" (str "Bearer " access-token)} :body {:id (:id mock) :content (:content mock) :sha (or (:sha mock) "") :git-repo (or (:git-repo mock) "")} :on-success [:app.dashboard/save-mock-success] :on-failure [:app.dashboard/save-mock-failed]} :db (assoc db :loading-edit-mock true)})))
|
@coderabbitai can you resolve all the outdated conversations, without commiting any changes, just resolve the chat? |
|
@Felipe-gsilva I'll resolve all outdated conversations in this PR. ✅ Actions performedComments resolved. |
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (7)
src/back/api/controllers/user.clj (2)
24-33: 🧹 Nitpick (assertive)Add docstring to clarify the new arity.
The function now supports an optional
install-idparameter but lacks documentation explaining its purpose and usage.
41-68: 🛠️ Refactor suggestionUse kebab-case consistently.
The functions use snake_case for parameters and keywords (e.g.,
git_username,:git_username) which is inconsistent with Clojure conventions.src/front/app/dashboard/events.cljs (1)
310-334:⚠️ Potential issueRemove debug prints and fix security concerns in push-mock handler.
The handler contains debug printing and sends unnecessary user data in the request.
src/back/api/controllers/sync.clj (4)
10-13:⚠️ Potential issueCorrect the typo in the commit message.
There's a typo in the commit message: "Auto genereted commit" should be "Auto generated commit".
-(str "Auto genereted commit by " author "!!\n" +(str "Auto generated commit by " author "!!\n"
41-58: 🛠️ Refactor suggestionUse consistent API URL and remove debugging statement.
The function has two issues:
- Uses hardcoded GitHub API URL instead of the passed parameter
- Contains debugging statement (
inspect)-(defn create-blob [gh-client install-id owner repo file] +(defn create-blob [gh-client install-id owner repo file github-api-url] (let [response (gh-app/request* gh-client install-id {:method :post - :url (inspect (format "%s/repos/%s/%s/git/blobs" "https://api.github.com" owner repo)) + :url (format "%s/repos/%s/%s/git/blobs" github-api-url owner repo)
69-86: 🛠️ Refactor suggestionRemove debugging statements from tree creation.
The function contains multiple debugging statements that should be removed.
- :body (inspect (json/encode {:base_tree (str base-tree-sha) + :body (json/encode {:base_tree (str base-tree-sha) ... - (let [content (inspect (-> response + (let [content (-> response
200-235:⚠️ Potential issueRemove commented code containing sensitive information.
The commented section contains sensitive information such as:
- Private keys
- Installation IDs
- Personal email addresses
This code should be removed to prevent accidental exposure of sensitive data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
src/back/api/adapters/customers.clj(1 hunks)src/back/api/adapters/mocks.clj(2 hunks)src/back/api/controllers/mocks.clj(2 hunks)src/back/api/controllers/sync.clj(1 hunks)src/back/api/controllers/user.clj(2 hunks)src/back/api/logic/mocks.clj(3 hunks)src/back/api/ports/http_in.clj(7 hunks)src/back/api/routes.clj(5 hunks)src/front/app/components/svg.cljs(9 hunks)src/front/app/dashboard/base.cljs(3 hunks)src/front/app/dashboard/editor.cljs(5 hunks)src/front/app/dashboard/events.cljs(3 hunks)src/front/app/dashboard/subs.cljs(3 hunks)
🧰 Additional context used
📓 Learnings (1)
src/back/api/ports/http_in.clj (1)
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#375
File: src/back/api/ports/http_in.clj:218-277
Timestamp: 2025-01-24T13:37:14.472Z
Learning: The webhook endpoint in moclojer-app verifies GitHub webhook signatures using the verify-webhook-signature interceptor from back.api.interceptors.verify-webhook, which uses clj-github-app.webhook-signature library.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: WIP
🔇 Additional comments (34)
src/front/app/dashboard/base.cljs (6)
2-15: Imports look consistent and well-organized.
No issues found with the new or updated:requirestatements.
98-98: Replacingdefncwithdefnmay cause issues in component definitions.
This was previously noted.
150-150: Possible typo in class name'lm:col-span-3'.
Previously noted.
161-161::on-loadevent handler on input element may not work as intended.
Previously noted.
176-177: Use:classinstead of:class-namefor DOM elements in Helix.
Previously noted.
321-334: Index component usage looks good.
No issues found. Thedefncmacro ensures proper hook usage if any are added later, and the layout is cohesive.src/back/api/logic/mocks.clj (5)
2-5: Namespace import structure looks good.
The multi-line:requireform is clear and makes each import easily identifiable.
59-59: Ensure consistent handling of new keys.
Adding:git-repoand:shato the selected keys is fine. However, confirm if these new fields may sometimes benilor empty strings and if further checks or validations are needed.
92-93: Consider validating SHA format.
To avoid storing invalid or non-40-character hex strings in:mock/sha, you could verify the input.(defn update-sha [mock sha] - (assoc mock :mock/sha sha)) + (when (re-matches #"^[0-9a-f]{40}$" sha) + (assoc mock :mock/sha sha)))
95-96: Consider validating repository URL.
Ifgit-repois intended to be a valid URL, you could add a basic check or rely on the calling code or schema validation.
98-99: Consider adding basic content checks.
Ifcontentis expected to be in a valid format or within certain size limits, you might add at least minimal validation to prevent issues downstream.src/back/api/ports/http_in.clj (7)
2-14: Namespace imports are well-organized.
The clearly listed dependencies make the code easier to maintain.
25-30: Check for nil install-id usage.
Ensure thatcontrollers.user/edit-user!handlesnilvalues forinstall-idgracefully and does not overwrite or remove existing data unintentionally.
65-66: Avoid debug-only functions in production code.
inspectappears to be for debugging. Remove it or guard it behind a debug flag to maintain a cleaner production codebase.
106-106: Validate user existence.
Ensurehandler-get-mockshandles cases whereget-user-by-idreturnsnil, especially if the user is not found or session data is invalid.
236-239: Use path functions for more robust filtering.
Splitting on"/"works, but usingjava.io.Fileor a dedicated path library helps avoid corner cases (Linux vs. Windows paths, etc.).
244-348: Refactor large webhook handler.
handler-webhookis quite long and handles multiple logical flows. Splitting into dedicated functions (push event flow, installation event handling, etc.) would aid readability and testing.
240-243:⚠️ Potential issueAvoid duplicative
inspectdefinitions.
You appear to have two implementations ofinspect. Consolidate them into one or remove if unneeded.- (defn inspect [a] (prn "inspeciona " a) a) - (defn inspect [a] - (prn "inspecionando " a (newline)) - a) + ;; Remove or guard behind a debug flagLikely invalid or redundant comment.
src/back/api/adapters/customers.clj (1)
6-14: Prevent “nil” as string and validate ID fields.
Using(str org_id)might produce"nil". A safer approach to avoid stringifyingnilis(when org_id (str org_id)). Similarly, add checks ifgit_install_idmust be positive or ifgit_usernamemust be non-empty:(-> {:uuid (str uuid) :email email :external-id external_uuid} - (assoc-if :org-id (str org_id)) + (assoc-if :org-id (when org_id (str org_id))) (assoc-if :username username) (assoc-if :git-install-id (when (pos? git_install_id) git_install_id)) (assoc-if :git-username git_username))src/front/app/dashboard/subs.cljs (3)
53-56: LGTM!The subscription follows the standard pattern and correctly retrieves the settings modal state.
95-98: LGTM!The subscription follows the standard pattern and correctly retrieves the Git repository requirement state.
100-103: LGTM!The subscription follows the standard pattern and correctly retrieves the repositories list.
src/back/api/controllers/mocks.clj (2)
13-15: LGTM! Performance improvement.Replacing
mergewithassocfor updating the logging context is a good optimization, asassocis more efficient thanmergefor adding key-value pairs.
65-67: LGTM! Performance improvement.Replacing
mergewithassocfor updating the logging context is a good optimization, asassocis more efficient thanmergefor adding key-value pairs.src/back/api/routes.clj (2)
157-158: LGTM! Consistent error handling.Adding the
error-handler-interceptorensures consistent error handling across routes.
197-202:⚠️ Potential issueReorder interceptors to verify webhook signature before parsing the request body.
The
verify-webhook-signatureinterceptor should be placed before theparameters/parameters-interceptorto ensure the raw request body is available for signature verification. Parsing the body before verification can compromise security.Apply this diff to reorder the interceptors:
- :interceptors [(parameters/parameters-interceptor) - (verify-webhook-signature)] + :interceptors [(verify-webhook-signature) + (parameters/parameters-interceptor)]⛔ Skipped due to learnings
Learnt from: Felipe-gsilva PR: moclojer/moclojer-app#375 File: src/back/api/ports/http_in.clj:218-277 Timestamp: 2025-01-24T13:37:14.472Z Learning: The webhook endpoint in moclojer-app verifies GitHub webhook signatures using the verify-webhook-signature interceptor from back.api.interceptors.verify-webhook, which uses clj-github-app.webhook-signature library.src/front/app/dashboard/editor.cljs (4)
Line range hint
75-82: LGTM! Enhanced user interaction feedback.Adding transition effects for hover states improves the user experience by providing visual feedback.
Line range hint
85-95: LGTM! Enhanced user interaction feedback.Adding transition effects for hover states improves the user experience by providing visual feedback.
97-103: LGTM! Well-designed component.The component follows the design system, includes transition effects, and correctly dispatches the remove action.
105-115: 🧹 Nitpick (assertive)Enhance Git operations UX.
The component could be improved in several ways:
- Add loading state handling
- Use a more appropriate icon for the push action
- Add tooltips for better user guidance
Apply this diff to enhance the component:
-(defnc push-button [{:keys [mock-id]}] - (let [mock-valid? (refx/use-sub [:app.dashboard/mock-valid?])] +(defnc push-button [{:keys [mock-id]}] + (let [mock-valid? (refx/use-sub [:app.dashboard/mock-valid?]) + loading? (refx/use-sub [:app.dashboard/loading-push-mock])] (d/button {:class-name (str "px-3 py-2 bg-gray-500 rounded-lg flex justify-center items-center btn-add space-x-2 " "hover:bg-gray-600 transition-all duration-75 " - (when-not mock-valid? " opacity-50 cursor-not-allowed")) + (when (or (not mock-valid?) loading?) " opacity-50 cursor-not-allowed")) + :title "Push changes to Git repository" + :disabled (or (not mock-valid?) loading?) :on-click #(when mock-valid? (refx/dispatch-sync [:app.dashboard/push-mock mock-id]) (refx/dispatch-sync [:app.dashboard/reset-mock-pub-stts mock-id]))} - (d/div {:class-name "text-white text-xs font-bold leading-[18px]"} " push to origin") - ($ svg/save)))) + (d/div {:class-name "text-white text-xs font-bold leading-[18px]"} + (if loading? "pushing..." " push to origin")) + (if loading? + ($ svg/spinner) + ($ svg/git-push)))))Likely invalid or redundant comment.
src/back/api/adapters/mocks.clj (1)
Line range hint
5-17: LGTM! Clean addition of git sync fields.The changes correctly add support for git sync related fields (sha and git_repo) while maintaining the existing pattern of using
assoc-iffor optional fields.src/front/app/dashboard/events.cljs (1)
268-279:⚠️ Potential issueFix incorrect event handler registration and debug logging.
The handler:
- Uses
reg-event-dbbut returns effects (:http)- Contains debug logging that should be removed
- Has incorrect URL construction
Apply this diff to fix the issues:
-(refx/reg-event-db +(refx/reg-event-fx :app.dashboard/set-new-username - (fn [{{:keys [current-user]} :db} [_ username-to-save]] - (js/console.log username-to-save current-user) + (fn [{db :db} [_ username-to-save]] (let [access-token (get-in db [:current-user :access-token]) user-id (get-in db [:current-user :user :user_id])] {:http {:url (str "/user/" user-id) :method :post :headers {"authorization" (str "Bearer " access-token)} :body {:username username-to-save} - :on-success [] - :on-failure []}}))) + :on-success [:app.dashboard/toggle-settings] + :on-failure [:app.dashboard/save-mock-failed]}})))Likely invalid or redundant comment.
src/front/app/components/svg.cljs (1)
Line range hint
34-334: LGTM! Clean SVG component updates.The changes:
- Consistently use the
classattribute instead ofclass-name- Add new SVG components (github, docs) following the established patterns
- Maintain consistent formatting and structure
src/back/api/controllers/sync.clj (1)
108-159: LGTM! Well-implemented reference and repository operations.The implementation includes:
- Proper error handling
- Consistent authentication
- Clear response processing
| (defn update-repo-modal [] | ||
| (let [require-git-repo? (refx/use-sub [:app.dashboard/require-git-repo?]) | ||
| repos (refx/use-sub [:app.dashboard/repos])] | ||
| ($ modal | ||
| {:title "Add a repo to your mock" | ||
| :open? require-git-repo? | ||
| :on-close #(refx/dispatch-sync [:app.dashboard/toggle-git-repo-modal require-git-repo?]) | ||
| :children | ||
| (d/div | ||
| {:class "w-screen md:w-[600px] p-6 gap-4 z-10"} | ||
| (d/div | ||
| {:class "w-[calc(100%)]"} | ||
| (d/ul | ||
| {:class "space-y-2"} | ||
| (for [repo repos] | ||
| (d/li | ||
| {:key (:full_name repo) | ||
| :class "flex items-center p-3 border rounded-lg hover:bg-gray-50 cursor-pointer" | ||
| :on-click #(do | ||
| (close-modal!) | ||
| (refx/dispatch-sync [:app.dashboard/delete-mock mock]))} | ||
| (d/p | ||
| {:class "text-white"} | ||
| "Yes, I want to delete it")) | ||
| ($ button | ||
| {:class-name "px-4 py-2 border-2 border-gray-400 rounded-lg hover:bg-gray-200"} | ||
| (d/p | ||
| {:class-name "text-gray-800 text-sm font-medium" | ||
| :on-click #(close-modal!)} | ||
| "No, cancel"))))}))) | ||
| (refx/dispatch-sync [:app.dashboard/update-git-repo (:html_url repo)]) | ||
| (refx/dispatch-sync [:app.dashboard/toggle-git-repo-modal require-git-repo?]))} | ||
| (d/img | ||
| {:src (get-in repo [:owner :avatar_url]) | ||
| :alt (str (get-in repo [:owner :login]) "'s avatar") | ||
| :class "w-8 h-8 rounded-full mr-3"}) | ||
| (d/span | ||
| {:class "flex-1"} | ||
| (:full_name repo)))))))}))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add a fallback message for empty repo lists.
If repos is empty, the modal remains blank. Consider displaying a “No repositories found” message for a better user experience.
src/front/app/dashboard/base.cljs
Outdated
| (d/div {:class "col-span-6 row-span-7 h-64 rounded-md bg-gray-50 space-y-4 p-4"} | ||
| (cond | ||
| (= (:view setting) "user") | ||
| (d/div | ||
| (d/form {:class "flex flex-col justify-between w-full items-start" | ||
| :on-submit (fn [e] | ||
| (.preventDefault e) | ||
| (when (and (not-empty username-to-save) available?) | ||
| (refx/dispatch [:app.dashboard/set-new-username username-to-save])))} | ||
| (d/div {:class "w-[calc(100%)]"} | ||
| ($ input {:label "Edit Username" | ||
| :on-change (fn [e] | ||
| (.preventDefault e) | ||
| (refx/dispatch-sync [:app.auth/username-available? (.. e -target -value)])) | ||
| :placeholder username}) | ||
| (d/p {:class "text-sm select-none "} | ||
| (str "<" username ">.moclojer.com"))) | ||
| (d/div {:class (str "w-[calc(20%)] flex justify-center items-center text-white rounded-md " | ||
| "my-2 p-2 ")} | ||
| ($ button {:class (when (not available?) "cursor-not-allowed ") | ||
| :theme :mockingbird | ||
| :padding :sm | ||
| :type :submit} | ||
| "Update")))) | ||
|
|
||
| (= (:view setting) "orgs") | ||
| (do | ||
| (d/div | ||
| (d/p "Work in Progress") | ||
| (if (> (count user-orgs) 1) | ||
| (doseq [org user-orgs] | ||
| (d/p org))) | ||
| (d/p user-orgs))) | ||
|
|
||
| (= (:view setting) "mocks") | ||
| (d/div | ||
| (d/p "Work in Progress")) | ||
| :else | ||
| (set-setting assoc :view "user")))))}))) |
There was a problem hiding this comment.
doseq inside the organizations view prevents rendering of org elements.
Within the "orgs" view, line 88 uses (doseq [org user-orgs] (d/p org)), which does not produce a sequence of DOM elements. To actually render the organizations, replace doseq with a mapping function (e.g., map or mapv).
- (doseq [org user-orgs]
- (d/p org))
+ (mapv (fn [org]
+ (d/p org))
+ user-orgs)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (d/div {:class "col-span-6 row-span-7 h-64 rounded-md bg-gray-50 space-y-4 p-4"} | |
| (cond | |
| (= (:view setting) "user") | |
| (d/div | |
| (d/form {:class "flex flex-col justify-between w-full items-start" | |
| :on-submit (fn [e] | |
| (.preventDefault e) | |
| (when (and (not-empty username-to-save) available?) | |
| (refx/dispatch [:app.dashboard/set-new-username username-to-save])))} | |
| (d/div {:class "w-[calc(100%)]"} | |
| ($ input {:label "Edit Username" | |
| :on-change (fn [e] | |
| (.preventDefault e) | |
| (refx/dispatch-sync [:app.auth/username-available? (.. e -target -value)])) | |
| :placeholder username}) | |
| (d/p {:class "text-sm select-none "} | |
| (str "<" username ">.moclojer.com"))) | |
| (d/div {:class (str "w-[calc(20%)] flex justify-center items-center text-white rounded-md " | |
| "my-2 p-2 ")} | |
| ($ button {:class (when (not available?) "cursor-not-allowed ") | |
| :theme :mockingbird | |
| :padding :sm | |
| :type :submit} | |
| "Update")))) | |
| (= (:view setting) "orgs") | |
| (do | |
| (d/div | |
| (d/p "Work in Progress") | |
| (if (> (count user-orgs) 1) | |
| (doseq [org user-orgs] | |
| (d/p org))) | |
| (d/p user-orgs))) | |
| (= (:view setting) "mocks") | |
| (d/div | |
| (d/p "Work in Progress")) | |
| :else | |
| (set-setting assoc :view "user")))))}))) | |
| (d/div {:class "col-span-6 row-span-7 h-64 rounded-md bg-gray-50 space-y-4 p-4"} | |
| (cond | |
| (= (:view setting) "user") | |
| (d/div | |
| (d/form {:class "flex flex-col justify-between w-full items-start" | |
| :on-submit (fn [e] | |
| (.preventDefault e) | |
| (when (and (not-empty username-to-save) available?) | |
| (refx/dispatch [:app.dashboard/set-new-username username-to-save])))} | |
| (d/div {:class "w-[calc(100%)]"} | |
| ($ input {:label "Edit Username" | |
| :on-change (fn [e] | |
| (.preventDefault e) | |
| (refx/dispatch-sync [:app.auth/username-available? (.. e -target -value)])) | |
| :placeholder username}) | |
| (d/p {:class "text-sm select-none "} | |
| (str "<" username ">.moclojer.com"))) | |
| (d/div {:class (str "w-[calc(20%)] flex justify-center items-center text-white rounded-md " | |
| "my-2 p-2 ")} | |
| ($ button {:class (when (not available?) "cursor-not-allowed ") | |
| :theme :mockingbird | |
| :padding :sm | |
| :type :submit} | |
| "Update")))) | |
| (= (:view setting) "orgs") | |
| (do | |
| (d/div | |
| (d/p "Work in Progress") | |
| (if (> (count user-orgs) 1) | |
| (mapv (fn [org] | |
| (d/p org)) | |
| user-orgs)) | |
| (d/p user-orgs))) | |
| (= (:view setting) "mocks") | |
| (d/div | |
| (d/p "Work in Progress")) | |
| :else | |
| (set-setting assoc :view "user")))))}))) |
| (defn enable-sync | ||
| "Updates user install id field" | ||
| [install-id user-id {:keys [database]} ctx] | ||
| (logs/log :info "enabling git sync" | ||
| :ctx (assoc ctx :user user-id)) | ||
| (or (-> {:customer/uuid (parse-uuid (str user-id))} | ||
| (logic.customers/update-install-id install-id) | ||
| (db.customers/update! database ctx) | ||
| (adapter.customers/->wire)) | ||
| (throw (ex-info "Could not enable git sync" | ||
| {:status-code 412 | ||
| :value (assoc ctx :user user-id)})))) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in enable-sync.
The function:
- Uses string concatenation with
strbeforeparse-uuidwhich is redundant ifuser-idis already a string - Doesn't specify the error cause in the exception
- Could benefit from validating the install-id format
Apply this diff to improve the implementation:
(defn enable-sync
"Updates user install id field"
[install-id user-id {:keys [database]} ctx]
+ (when-not (re-matches #"^\d+$" install-id)
+ (throw (ex-info "Invalid GitHub installation ID format"
+ {:status-code 400
+ :cause :invalid-install-id
+ :value (assoc ctx :install-id install-id)})))
(logs/log :info "enabling git sync"
- :ctx (assoc ctx :user user-id))
+ :ctx (assoc ctx :user-id user-id :install-id install-id))
- (or (-> {:customer/uuid (parse-uuid (str user-id))}
+ (or (-> {:customer/uuid (parse-uuid user-id)}
(logic.customers/update-install-id install-id)
(db.customers/update! database ctx)
(adapter.customers/->wire))
(throw (ex-info "Could not enable git sync"
{:status-code 412
+ :cause :git-sync-failed
:value (assoc ctx :user user-id)}))))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (defn enable-sync | |
| "Updates user install id field" | |
| [install-id user-id {:keys [database]} ctx] | |
| (logs/log :info "enabling git sync" | |
| :ctx (assoc ctx :user user-id)) | |
| (or (-> {:customer/uuid (parse-uuid (str user-id))} | |
| (logic.customers/update-install-id install-id) | |
| (db.customers/update! database ctx) | |
| (adapter.customers/->wire)) | |
| (throw (ex-info "Could not enable git sync" | |
| {:status-code 412 | |
| :value (assoc ctx :user user-id)})))) | |
| (defn enable-sync | |
| "Updates user install id field" | |
| [install-id user-id {:keys [database]} ctx] | |
| (when-not (re-matches #"^\d+$" install-id) | |
| (throw (ex-info "Invalid GitHub installation ID format" | |
| {:status-code 400 | |
| :cause :invalid-install-id | |
| :value (assoc ctx :install-id install-id)}))) | |
| (logs/log :info "enabling git sync" | |
| :ctx (assoc ctx :user-id user-id :install-id install-id)) | |
| (or (-> {:customer/uuid (parse-uuid user-id)} | |
| (logic.customers/update-install-id install-id) | |
| (db.customers/update! database ctx) | |
| (adapter.customers/->wire)) | |
| (throw (ex-info "Could not enable git sync" | |
| {:status-code 412 | |
| :cause :git-sync-failed | |
| :value (assoc ctx :user user-id)})))) |
| (defn get-by-git-username | ||
| [git_username {:keys [database]} ctx] | ||
| (logs/log :info "retrieving user by its git_username" | ||
| :ctx (assoc ctx :git_username git_username)) | ||
| (-> (db.customers/get-by-git-username git_username database ctx) | ||
| (adapter.customers/->wire))) |
There was a problem hiding this comment.
Function may throw NPE when user not found.
The function doesn't handle the case where no user is found with the given git username, unlike similar functions that follow.
Apply this diff to add proper error handling:
(defn get-by-git-username
[git_username {:keys [database]} ctx]
(logs/log :info "retrieving user by its git_username"
:ctx (assoc ctx :git_username git_username))
- (-> (db.customers/get-by-git-username git_username database ctx)
- (adapter.customers/->wire)))
+ (or (-> (db.customers/get-by-git-username git_username database ctx)
+ (adapter.customers/->wire))
+ (throw (ex-info "No user with given git username was found"
+ {:status-code 412
+ :cause :invalid-git-username
+ :value (assoc ctx :git-username git_username)}))))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (defn get-by-git-username | |
| [git_username {:keys [database]} ctx] | |
| (logs/log :info "retrieving user by its git_username" | |
| :ctx (assoc ctx :git_username git_username)) | |
| (-> (db.customers/get-by-git-username git_username database ctx) | |
| (adapter.customers/->wire))) | |
| (defn get-by-git-username | |
| [git_username {:keys [database]} ctx] | |
| (logs/log :info "retrieving user by its git_username" | |
| :ctx (assoc ctx :git_username git_username)) | |
| (or (-> (db.customers/get-by-git-username git_username database ctx) | |
| (adapter.customers/->wire)) | |
| (throw (ex-info "No user with given git username was found" | |
| {:status-code 412 | |
| :cause :invalid-git-username | |
| :value (assoc ctx :git-username git_username)})))) |
There was a problem hiding this comment.
Actionable comments posted: 14
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/back/api/ports/http_in.clj(7 hunks)src/back/api/routes.clj(4 hunks)src/front/app/dashboard/editor.cljs(6 hunks)src/front/app/dashboard/events.cljs(3 hunks)src/front/app/dashboard/subs.cljs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: WIP
🔇 Additional comments (23)
src/back/api/routes.clj (2)
67-67: LGTM! Good use of schema validation.The change from a generic map to
schemas.wire-in/UserUpdateimproves type safety and documentation.
32-38:⚠️ Potential issueReorder interceptors for secure webhook signature verification.
The
verify-webhook-signatureinterceptor should be placed before theparameters/parameters-interceptorto ensure the raw request body is available for signature verification.Apply this diff to fix the interceptor order:
{:post {:summary "Handles a webhook" :parameters {:body map?} ;; TODO check if this can be removed :interceptors [(parameters/parameters-interceptor) - (verify-webhook-signature)] + (verify-webhook-signature)]}Also, please clarify or remove the TODO comment about interceptor removal. The
parameters/parameters-interceptoris required to parse the request body into a Clojure map.✅ Verification successful
Confirmed: Interceptor order must be changed for secure webhook signature verification
The implementation shows that
verify-webhook-signaturerequires access to the raw request body to verify the signature. The current order allowsparameters-interceptorto modify the body before signature verification, which would cause verification to fail. The suggested reordering is correct.
- In
src/back/api/routes.clj, reorder interceptors to putverify-webhook-signaturefirst- The TODO comment can be safely removed as both interceptors serve distinct, necessary purposes:
verify-webhook-signature: Validates request authenticityparameters-interceptor: Parses the request body into Clojure data structures🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for verify-webhook-signature implementation echo "=== Searching for verify-webhook-signature implementation ===" rg -A 10 "verify-webhook-signature" --type clojure echo -e "\n=== Searching for webhook signature verification tests ===" rg -A 10 "verify-webhook-signature" --type clojure --glob "*test*" echo -e "\n=== Searching for parameters-interceptor usage and documentation ===" rg "parameters-interceptor" --type clojureLength of output: 2957
Script:
#!/bin/bash # Search for webhook signature verification implementation echo "=== Searching for webhook-signature namespace ===" rg -A 10 "check-payload-signature-256" --type clojure echo -e "\n=== Looking for parameters interceptor implementation ===" fd -e clj -e cljc -H parametersLength of output: 1257
src/back/api/ports/http_in.clj (4)
25-30: LGTM! Function signature matches schema update.The addition of
install-idparameter aligns with the schema changes inroutes.clj.
65-65:⚠️ Potential issueRemove debug function from production code.
The
inspectfunction usesprnfor debugging and should be removed from production code.Apply this diff:
-(defn inspect [a] (prn "inspeciona " a) a)Likely invalid or redundant comment.
237-240: 🧹 Nitpick (assertive)Use path manipulation functions for robustness.
String operations for path manipulation can be fragile. Consider using proper path manipulation functions.
Apply this diff:
-(defn- filter-mocks [files] - (into [] (filter #(and (= (last (str/split % #"/")) "moclojer.yml") - (str/includes? % "mocks/")) - files))) +(defn- filter-mocks [files] + (let [mock-filename "moclojer.yml" + mock-dir "mocks"] + (into [] (filter #(let [path (java.nio.file.Paths/get % (into-array String []))] + (and (= (.getFileName path) mock-filename) + (.startsWith path mock-dir))) + files))))Likely invalid or redundant comment.
68-101:⚠️ Potential issueImprove path manipulation and error handling.
Several issues need attention:
- String splitting for git path is fragile
- Missing error handling for path manipulation
- Unaddressed TODO comment
Apply these improvements:
-(let [owner (inspect (-> (str/split git-path #"/") - (as-> [e] (take-last 2 e)) - (first)))] +(let [path-parts (str/split git-path #"/") + owner (try + (-> path-parts + reverse + second) + (catch Exception e + (throw (ex-info "Invalid git repository path" + {:status-code 400 + :cause :invalid-git-path + :value git-path}))))]Also, please clarify the TODO comment about org/user check or implement the necessary logic.
✅ Verification successful
Confirmed: Path handling needs improvement and error handling standardization
The review suggestions are valid and align with existing patterns in the codebase. The proposed changes should:
- Follow the established error handling pattern:
(throw (ex-info "Invalid git repository path" {:status-code 400 :cause :invalid-git-path :value git-path}))
The TODO comment about org/user check is valid as the codebase handles both types differently in controllers. This distinction needs to be implemented.
Path manipulation should be standardized across the codebase, as currently there are inconsistent approaches.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for git path handling patterns echo "=== Git Path Handling Patterns ===" rg -A 2 "str/split.*#\"/\"" echo -e "\n=== Error Handling Patterns ===" # Search for similar error handling patterns with ex-info rg "throw.*ex-info.*:invalid-git" echo -e "\n=== Org/User Check Patterns ===" # Search for org/user check implementations rg -B 2 -A 2 "check.*org.*user|org.*user.*check"Length of output: 2023
Script:
#!/bin/bash # Search for general error handling patterns with ex-info echo "=== Error Handling Patterns ===" rg "throw.*ex-info.*:status-code 400" -A 2 echo -e "\n=== Organization vs User Implementation ===" # Search for organization/user related code in controllers rg -l "org.*user|organization.*user" src/back/controllers/Length of output: 343
Script:
#!/bin/bash # Find directory structure echo "=== Directory Structure ===" fd -t d controllers echo -e "\n=== Error Handling Patterns ===" # Broader search for error handling rg "throw.*ex-info" -A 2 echo -e "\n=== Controller Files ===" # Find controller files fd -t f -e clj ".*controller.*"Length of output: 6839
src/front/app/dashboard/events.cljs (5)
2-3: No issues spotted in the require statement.
24-25: Toggle logic looks good.
268-280: Incorrect usage ofreg-event-db; should usereg-event-fx.
This handler returns HTTP effects but is usingreg-event-db. This is a duplicate of a past comment regarding mixing effects andreg-event-db. Ensure you return a:dbkey alongside your effects or switch toreg-event-fx.-(refx/reg-event-db +(refx/reg-event-fx :app.dashboard/set-new-username (fn [{db :db} [_ username-to-save]] (js/console.log username-to-save current-user) (let [access-token (:access-token current-user) user-id (:user_id (:user current-user))] {:db db :http {:url (str "user/" user-id) :method :post :headers {"authorization" (str "Bearer " access-token)} :body {:username username-to-save} :on-success [] :on-failure []}})))
300-309: Storing repositories looks good.
286-298: Verify correctness of pushing the mock after clearing:curr-mock-id.
on-successtriggers:app.dashboard/push-mockwith the old:curr-mock-id, but you set:curr-mock-idtonilin the same effect. This could cause confusion if subsequent handlers rely on that ID. Please confirm it behaves as intended.src/front/app/dashboard/subs.cljs (5)
2-3: No issues with updated require statement.
53-57: Subscription logic foris-settings-open?is good.
95-99: Subscription torequire-git-repo?is straightforward.
100-104: Storing repos in:repositoriesis fine.
105-108:is-sync-enabled?subscription looks good.src/front/app/dashboard/editor.cljs (7)
1-2: Namespace configuration is acceptable.
75-75: Styling update is fine.
85-85: File upload button handling is fine.
95-96: No issues with the final part of the file-upload block.
105-116: Push button logic is correctly disabling interaction when invalid.
141-142: Retrievingmock-to-deleteandsync-enabled?looks fine.
203-207: Conditional push vs. enable sync is correct.
| (refx/reg-event-db | ||
| :app.dashboard/toggle-git-repo-modal | ||
| (fn [db [_ modal-open?]] | ||
| (assoc db :require-git-repo? (not modal-open?)))) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Confirm naming consistency.
Toggling the git-repo modal inverts :require-git-repo?. Verify that this accurately reflects the desired logic, as the naming can be confusing.
There was a problem hiding this comment.
Actionable comments posted: 25
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
src/back/api/controllers/mocks.clj(2 hunks)src/back/api/controllers/sync.clj(1 hunks)src/back/api/ports/http_in.clj(8 hunks)src/back/api/routes.clj(6 hunks)src/back/api/schemas/wire_in.clj(2 hunks)src/back/api/utils.clj(1 hunks)src/front/app/auth/supabase.cljs(3 hunks)src/front/app/auth/views.cljs(2 hunks)src/front/app/components/svg.cljs(9 hunks)src/front/app/dashboard/editor.cljs(7 hunks)src/front/app/dashboard/events.cljs(5 hunks)src/front/app/dashboard/mocks.cljs(4 hunks)src/front/app/dashboard/subs.cljs(3 hunks)
🧰 Additional context used
📓 Learnings (1)
src/back/api/ports/http_in.clj (1)
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#375
File: src/back/api/ports/http_in.clj:218-277
Timestamp: 2025-01-24T13:37:14.472Z
Learning: The webhook endpoint in moclojer-app verifies GitHub webhook signatures using the verify-webhook-signature interceptor from back.api.interceptors.verify-webhook, which uses clj-github-app.webhook-signature library.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: WIP
🔇 Additional comments (51)
src/front/app/components/svg.cljs (2)
318-324: Well-structured new components with proper styling!The new
githubanddocscomponents follow best practices:
- Consistent styling with other icons
- Proper dark mode support
- Responsive sizing with flex utilities
Also applies to: 326-332
Line range hint
1-334: Overall well-structured SVG components!The file demonstrates good practices:
- Consistent component structure
- Proper accessibility attributes
- Dark mode support
- Responsive design considerations
src/front/app/dashboard/subs.cljs (1)
54-58: LGTM! Clean subscription implementations.The new subscriptions follow re-frame/refx patterns correctly and implement simple database lookups.
Also applies to: 96-100, 101-105, 106-110, 111-115
src/front/app/auth/supabase.cljs (4)
11-12: Add a check forSUPABASE_OAUTH_REDIRECTto prevent undefined redirection issuesThe validation check should include
SUPABASE_OAUTH_REDIRECTto ensure it's properly set.
39-41: Simplify the construction of options by removing unnecessaryclj->jscallThe
:optionskey is being converted to a JavaScript object twice.
67-68: Add comprehensive test coverage for authentication flowsWhile the comment block provides useful examples, proper test coverage is needed.
36-37: Consider expanding supported OAuth providersThe provider validation is currently limited to GitHub only. If there are plans to support additional providers in the future (e.g., GitLab, Bitbucket), consider:
- Documenting this limitation
- Making the provider list configurable
Run this script to check for other OAuth providers in the codebase:
✅ Verification successful
Provider Validation Matches Current Usage
The search confirms that only "github" is being referenced (e.g., in
src/front/app/auth/views.cljs). As no other providers (e.g., GitLab or Bitbucket) are used, the strict provider validation insrc/front/app/auth/supabase.cljsis consistent with the current implementation. If additional providers are planned, consider documenting this limitation or making the provider list configurable.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for OAuth provider references rg -i "provider.*(?:github|gitlab|bitbucket)" --type clojureLength of output: 213
src/front/app/auth/views.cljs (6)
3-5: Great use of mockingbird UI components.The imports for
button,image, andinputappear valid and properly referenced. This integration simplifies the code and increases consistency with pre-built, well-tested components.
11-11: SVG import looks appropriate.Importing the
githubcomponent here is clear and consistent, ensuring that the GitHub icon is readily available without repeatedly defining SVG assets in-line.
60-63: Clarify the login method selection logic when email is blank.Currently, the logic triggers an email-based login whenever
91-99: Ensure consistent state management when switching to OAuth login.The
on-clickhandler sets:providerto"github"but does not reset or remove
145-146: Implement debouncing for username availability checks.Checking username availability on every keystroke can cause frequent backend calls and potential performance overhead. Consider wrapping the dispatch in a debounce utility to reduce extraneous calls while the user is typing.
160-162: Simplify the:disabledattribute expression.Replacing the
ifstatement with the direct boolean expression can make the code more concise and readable. For instance:- :disabled (if (or loading? (not available?)) - true - false) + :disabled (or loading? (not available?))src/back/api/utils.clj (5)
1-3: LGTM!The namespace declaration and imports are appropriate for the functionality provided by this utility namespace.
5-11: Enhance docstring clarity.The docstring could be more specific about what constitutes a valid value (truthy vs. falsy in Clojure).
13-18: Add input validation and error handling for decode function.The function should handle edge cases and malformed input safely, especially important for webhook payloads.
26-31: Optimize validation order and enhance docstring.The validation checks can be reordered for better performance, and the docstring could be more descriptive.
20-24:⚠️ Potential issueImprove input validation for encode function.
While the function attempts to handle non-string inputs with
str, it should still include proper validation and error handling.Apply this diff to improve the implementation:
-(defn encode [content] - (let [content-str (str content) - bytes (.getBytes content-str "UTF-8") - encoder (Base64/getEncoder)] - (.encodeToString encoder bytes))) +(defn encode + "Encodes a given value into base64 format after converting it to string" + [content] + (try + (let [content-str (str content) + bytes (.getBytes content-str "UTF-8") + encoder (Base64/getEncoder)] + (.encodeToString encoder bytes)) + (catch Exception e + (throw (ex-info "Failed to encode to base64" + {:type :encode-error + :value content + :cause (.getMessage e)})))))Likely invalid or redundant comment.
src/front/app/dashboard/editor.cljs (6)
1-18: No immediate concerns with the namespace and imports
All required dependencies appear correctly imported, and the namespace declaration is consistent with the rest of the dashboard code.
31-36: Effectful polling logic verified
The use-effect hook properly starts and stops mock polling based on theid, and the cleanup function is returned correctly.
75-79: Drag-drop appearance looks good
No issues identified in the presentation and structure of the drop zone elements.
107-114: Use standard Tailwind color scale
Tailwind typically expects classes likehover:bg-red-500rather thanhover:bg-red.
237-239: Toolbar and file upload integration
No issues found. The integration ofeditor-toolbarandfile-upload-buttonis straightforward and appears correct.
240-250: Drag-drop and editor usage
The drag-drop flow is well orchestrated and the editor hooking looks consistent.src/front/app/dashboard/mocks.cljs (2)
2-12: LGTM! Clean import organization.The imports are well-organized and the addition of the Mockingbird button component aligns with the modernization effort.
Line range hint
96-118: Well-structured component with proper hooks usage.The implementation follows React/Helix best practices with proper hooks usage for data fetching and state management. The auto-opening of the mock modal for empty states is a good UX touch.
src/back/api/controllers/sync.clj (3)
11-11: Correct the typographical error in the commit message.The string
"Auto genereted commit by"should be changed to"Auto generated commit by".- (str "Auto genereted commit by " author "!!\n" + (str "Auto generated commit by " author "!!\n" "Co-authored-by: " co-author " <" email ">"))
217-243: Remove or secure commented-out code containing sensitive information.This block includes references to private keys, installation IDs, and other sensitive data. It should be removed or securely stored to avoid accidental exposure.
43-43: 🛠️ Refactor suggestionUse the configured GitHub API URL instead of the hard-coded URL.
In
create-blob, the URL is currently hard-coded to"https://api.github.com". Usegithub-api-urlfrom the function’s parameters or config to ensure consistency and to enable custom GitHub endpoints if needed.- :url (format "%s/repos/%s/%s/git/blobs" "https://api.github.com" owner repo) + :url (format "%s/repos/%s/%s/git/blobs" github-api-url owner repo)Likely invalid or redundant comment.
src/back/api/schemas/wire_in.clj (2)
5-6: Add format validation for GitHub username.While
:git-usernameis optional and typed asstring?, consider adding a regex or a predicate to validate the GitHub username format. This helps ensure data correctness and consistency.
42-43: Enforce positivity checks ongit-install-id.
[:git-install-id {:optional true} integer?]might allow negative values. Consider a validation rule to ensure the value is non-negative (or strictly positive if required).src/back/api/controllers/mocks.clj (2)
13-15: Logging context looks good.The approach of associating
:mockand:user-idinto the logging context is consistent and helpful for debugging. No issues found here.
43-50: Add SHA validation when updating the mock.Currently, the code updates the
shafield without verifying if it’s a valid 40-character hex string. Adding a check ensures data integrity and avoids unexpected errors downstream.src/front/app/dashboard/events.cljs (13)
311-319: LGTM!The handler correctly updates the db with repository data.
321-329: LGTM!The handler correctly initiates the Git sync check and sets loading state.
331-343: LGTM!The handler correctly updates sync status and shows appropriate notifications based on the response.
345-352: LGTM!The handler correctly handles the failure case by updating sync status and showing an error notification.
381-386: LGTM!The handler correctly updates loading state and shows a success notification.
397-401: LGTM!The handler correctly initiates mock polling.
403-406: LGTM!The handler correctly stops mock polling.
408-418: LGTM!The handler correctly implements mock polling with a 5-second interval.
429-435: LGTM!The handler correctly updates server mock data with appropriate validation.
437-445: LGTM!The handler correctly reloads mock data from the server.
286-289: Verify naming consistency.The handler name and parameter suggest toggling a modal, but the db key is
:require-git-repo?. This naming inconsistency could lead to confusion.Run the following script to verify the usage of these terms:
273-284:⚠️ Potential issueFix incorrect event handler registration.
The event handler is registered using
reg-event-dbbut returns an HTTP effect. When an event handler produces effects, it should be registered usingreg-event-fx.Apply this diff to fix the registration:
-(refx/reg-event-db +(refx/reg-event-fx :app.dashboard/set-new-username (fn [{{:keys [current-user]} :db} [_ username-to-save]] (let [access-token (:access-token current-user) user-id (:user_id (:user current-user))] {:http {:url (str "user/" user-id) :method :post :headers {"authorization" (str "Bearer " access-token)} :body {:username username-to-save} :on-success [:app.auth/save-username] :on-failure []}})))Likely invalid or redundant comment.
291-309: 🛠️ Refactor suggestionAdd loading state cleanup on failure.
The handler sets loading state but doesn't clean it up on failure.
Apply this diff to clean up loading state:
(refx/reg-event-fx :app.dashboard/update-git-repo (fn [{db :db} [_ git-repo]] (let [mock-id (:curr-mock-id db) mock (->> db :mocks-raw (filter #(= (str (:id %)) (str mock-id))) first)] {:http {:url "/mocks" :method :put :headers {"authorization" (str "Bearer " (-> db :current-user :access-token))} :body {:id mock-id :git-repo git-repo :content (:content mock)} :on-success [:app.dashboard/save-mock-success] - :on-failure [:app.dashboard/save-mock-failed]} + :on-failure [:app.dashboard/update-git-repo-failed]} :db (-> db (assoc :loading-edit-mock true) (dissoc :curr-mock-id))}))) +(refx/reg-event-db + :app.dashboard/update-git-repo-failed + (fn [db _] + (-> db + (assoc :loading-edit-mock false) + (dissoc :curr-mock-id))))Likely invalid or redundant comment.
src/back/api/ports/http_in.clj (5)
356-366: LGTM!The function correctly checks Git sync status based on the installation ID.
368-417: LGTM!The function correctly handles pushing mock changes to GitHub with appropriate error handling and logging.
419-428: Move example code to tests.Example code in comment blocks should be moved to test files for better maintainability and to ensure it stays up to date.
[ref
236-239: 🧹 Nitpick (assertive)Use path manipulation functions.
Instead of using string operations for path handling, consider using proper path manipulation functions.
Apply this diff to improve path handling:
(defn- filter-mocks [files] - (into [] (filter #(and (= (last (str/split % #"/")) "moclojer.yml") - (str/includes? % "mocks/")) - files))) + (let [mock-filename "moclojer.yml" + mock-dir "mocks"] + (into [] (filter #(and (= (.getName (java.io.File. %)) mock-filename) + (.startsWith % (str mock-dir "/"))) + files))))Likely invalid or redundant comment.
345-354: 🧹 Nitpick (assertive)Enhance error messages for better debugging.
The error message could be more descriptive about why repos couldn't be retrieved.
Apply this diff to improve error messages:
{:status 404 - :body {:message "Could not retrieve user repos"}}))) + :body {:message "Could not retrieve user repos: Git installation ID not found" + :user_id (:uuid user)}})))Likely invalid or redundant comment.
| (refx/reg-event-fx | ||
| :app.dashboard/push-mock-failed | ||
| (fn [{db :db} [_ response]] | ||
| (js/console.error "Push failed:", (clj->js response)) | ||
| {:db (assoc db :loading-push false) | ||
| :notification {:type :error | ||
| :content (or (-> response :body :message) | ||
| "Failed to push to GitHub")}})) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove debug print statement.
Debug print statements should not be in production code.
Apply this diff to remove the debug print:
- (js/console.error "Push failed:", (clj->js response))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (refx/reg-event-fx | |
| :app.dashboard/push-mock-failed | |
| (fn [{db :db} [_ response]] | |
| (js/console.error "Push failed:", (clj->js response)) | |
| {:db (assoc db :loading-push false) | |
| :notification {:type :error | |
| :content (or (-> response :body :message) | |
| "Failed to push to GitHub")}})) | |
| (refx/reg-event-fx | |
| :app.dashboard/push-mock-failed | |
| (fn [{db :db} [_ response]] | |
| {:db (assoc db :loading-push false) | |
| :notification {:type :error | |
| :content (or (-> response :body :message) | |
| "Failed to push to GitHub")}})) |
| (refx/reg-event-db | ||
| :app.dashboard/set-new-username | ||
| (fn [{{:keys [current-user]} :db} [_ username-to-save]] | ||
| (js/console.log username-to-save current-user) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove debug print statement.
Debug print statements should not be in production code.
Apply this diff to remove the debug print:
- (js/console.log username-to-save current-user)| mock (->> db :mocks-raw | ||
| (filter #(= (str (:id %)) (str mock-id))) | ||
| first)] | ||
| (js/console.log "Updating git repo with:" git-repo) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove debug print statement.
Debug print statements should not be in production code.
Apply this diff to remove the debug print:
- (js/console.log "Updating git repo with:" git-repo)| (js/console.log "Mock being pushed:" (clj->js mock)) | ||
| (js/console.log "Content type:" (type (:content mock))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove debug print statements.
Debug print statements should not be in production code.
Apply this diff to remove the debug prints:
- (js/console.log "Mock being pushed:" (clj->js mock))
- (js/console.log "Content type:" (type (:content mock)))
|
@avelino @J0sueTM for you guys to know, the user can now use git sync, but not in prod, yet, because we will need to add a few things first (I mentioned it in #379 #378 #381 specially), but it is working within dev context (hit me out in discord so I can explain how to set things up, since we need some secrets for handling git ops however, orgs are yet no perfectly implemented in UI (as it should already be #377), for that I'm sorry, because things got a little messy with the scale of this feature here 🙏 if anything seems off, just tag me here and I will explain it better for you all |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/back/api/ports/http_in.clj (1)
241-344: 🧹 Nitpick (assertive)Consider concurrency and multiple matches in the push-event logic.
- For large commits, processing each file in
doseqmight be slow. Evaluate whether parallel or batched operations could help.- Lines 294–309: If multiple existing mocks match the same
wildcard, consider clarifying how duplicates should be handled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/back/api/adapters/orgs.clj(1 hunks)src/back/api/controllers/orgs.clj(2 hunks)src/back/api/logic/orgs.clj(2 hunks)src/back/api/ports/http_in.clj(8 hunks)
🧰 Additional context used
📓 Learnings (1)
src/back/api/ports/http_in.clj (1)
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#375
File: src/back/api/ports/http_in.clj:218-277
Timestamp: 2025-01-24T13:37:14.472Z
Learning: The webhook endpoint in moclojer-app verifies GitHub webhook signatures using the verify-webhook-signature interceptor from back.api.interceptors.verify-webhook, which uses clj-github-app.webhook-signature library.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: WIP
🔇 Additional comments (10)
src/back/api/adapters/orgs.clj (2)
1-2: LGTM! Good choice of utility function.The addition of
assoc-iffrom utils is appropriate for handling conditional key association.
5-13: Verify Git-related field usage across the codebase.The function modifications look good. The conditional association of Git fields is implemented correctly, maintaining consistent naming conventions.
Let's verify the usage of these new fields:
✅ Verification successful
Git-related field naming and usage are consistent across the codebase.
The search results confirm that the new fields (
git-install-idandgit-orgname) and their corresponding snake_case forms (git_install_id, etc.) are used consistently across the adapters, schemas, controllers, and database logic. No naming discrepancies or typos were detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for git-install-id and git-orgname usage to ensure consistent naming rg -l "git-install-id|git_install_id|git-orgname|git_orgname" --type clojure # Check for any potential typos or inconsistent naming rg -i "git.*install.*id|git.*org.*name" --type clojureLength of output: 2950
src/back/api/ports/http_in.clj (6)
2-15: Imports look consistent and appropriate.
No issues found with the required namespaces. The dependencies match up with the usage throughout the file.
26-31: Handle potential edge cases forinstall-id.
Ifinstall-idis provided, ensure it’s a valid value. You might also handle scenarios whereinstall-idis absent or fails validation, returning an error message or status code.
89-91: Remove debug print statements from production code.
The(prn mocks)call appears to be for debugging or inspection and might accidentally expose details in production logs.
236-239: Use a more robust path manipulation approach.
Manually splitting file paths withstr/splitcan be brittle. If feasible, consider using a path library or a more standardized approach to avoid edge cases.
368-417: Ensure robust parsing forgit-repoinhandler-push-mock!.
When splittinggit-repoto derive[owner repo-name], verify that the resulting vector indeed has two parts. If the user supplies an unexpected or invalid URL, a runtime error may occur.
172-177: 🧹 Nitpick (assertive)Validate organization creation and membership more robustly.
- Lines 172–177: Consider concurrency or conflict checks for
handler-create-orgif multiple requests create the same org simultaneously.- Line 224: Ensure
new-org-useris nevernil; otherwise,(:user-id new-org-user)might fail.Also applies to: 224-224
src/back/api/controllers/orgs.clj (2)
57-60: LGTM!The changes to logging context in
add-org-user!andremove-org-user!functions look good. Usingassocinstead ofmergemakes the context handling clearer.Also applies to: 71-73
80-85:⚠️ Potential issueFix inconsistent function name and add error handling.
The function has two issues:
- The database function call uses a different name (
get-by-git-org-namevsgit-orgname).- Missing error handling for non-existent organizations.
Apply this diff to fix the issues:
(defn get-by-git-orgname [git-orgname {:keys [database]} ctx] (logs/log :info "retrieving org by its git_orgname" (assoc ctx :git-orgname git-orgname)) - (-> (db.orgs/get-by-git-org-name git-orgname database ctx) - (adapter.orgs/->wire))) + (or (-> (db.orgs/get-by-git-org-name git-orgname database ctx) + (adapter.orgs/->wire)) + (throw (ex-info "No organization with given Git organization name was found" + {:status-code 412 + :cause :invalid-git-orgname + :value git-orgname}))))Likely invalid or redundant comment.
|
@coderabbitai give us a brief summary about what has been changed in this PR |
|
Tip For best results, initiate chat on the files or code changes. @Felipe-gsilva Here's a summary of the key changes in this PR:
Note: As mentioned, while the feature is functional in development, it requires additional work before production deployment, including proper organization UI implementation (#377) and other related issues (#379, #378, #381). |
@coderabbitai you can be more specific over this answer, I said brief but I meant, not that brief |
|
Tip For best results, initiate chat on the files or code changes. @Felipe-gsilva Here's a more detailed breakdown of the changes in this PR:
The implementation allows for:
Development status:
|
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/back/api/ports/http_in.clj(8 hunks)src/front/app/dashboard/base.cljs(3 hunks)src/front/app/dashboard/editor.cljs(6 hunks)src/front/app/dashboard/events.cljs(5 hunks)src/front/app/dashboard/subs.cljs(3 hunks)src/front/app/routes/bookmarks.cljs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/back/api/ports/http_in.clj (1)
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#375
File: src/back/api/ports/http_in.clj:218-277
Timestamp: 2025-01-24T13:37:14.472Z
Learning: The webhook endpoint in moclojer-app verifies GitHub webhook signatures using the verify-webhook-signature interceptor from back.api.interceptors.verify-webhook, which uses clj-github-app.webhook-signature library.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: WIP
🔇 Additional comments (51)
src/front/app/routes/bookmarks.cljs (2)
2-6: Improved Indentation in:requireBlock.
The adjusted indentation in the:requireblock enhances readability and aligns with common Clojure style practices. No functionality has changed.
8-43: Clear and Consistent Route Definitions.
The route definitions remain clear and consistent. The structure for each route—including the naming, view mapping, and public accessibility—is well maintained.src/front/app/dashboard/base.cljs (6)
17-61: Consider usingdefncinstead ofdefnfor React components using Helix hooks.This file defines
settings-modalwithdefnbut internally uses Helix hooks (hooks/use-state). In Helix,defncis typically preferred to ensure correct hook usage and name resolution during macro expansion.
88-89:doseqdoesn't render DOM elements in React.Using
(doseq [org user-orgs] (d/p org))inside the return expression won't produce any rendered elements. Convert this to a mapping function (e.g.,mapv) to display the organizations.
150-150: Possible typo in class name'lm:col-span-3'.Did you intend
sm:col-span-3or another breakpoint? Helix doesn't recognize'lm:col-span-3'as a standard class name.
161-161::on-loadevent oninputis not supported in React.Consider removing
:on-loadand using ause-effecthook or:default-valuefor initialization.
176-177: Inconsistent use ofclassvsclass-name.You’re using
:class-namehere but:classelsewhere. In Helix, DOM elements typically expect the:classattribute.
292-335: Consider usingdefncif you are calling Helix hooks withinupdate-repo-modal.Helix’s
defncmacro is recommended for function components that utilize hooks for state or effect management.src/front/app/dashboard/events.cljs (27)
2-2: No issues found in import changes.
26-31: Looks good.Toggling the settings modal is implemented as a side-effecting event with
reg-event-fx, returning a new:dbstate.
39-45: No issues found.
113-120: Implementation is clean and consistent.
122-130: No apparent problems with the error handling logic here.
145-145: No issues found.
160-160: No issues found.
271-277: No concerns here.
279-283: No issues with the failure event logic.
285-286: No issues with the event parameters.
287-287: Remove debug print statement.Debug print statements (
js/console.log) should not remain in production code.
297-301: Confirm naming consistency for toggling the git-repo modal.Inverting
:require-git-repo?by passingmodal-open?might be confusing. Please verify.
303-321: Logic appears fine.
323-330: No issues found.
333-347: Implementation looks correct for enabling Git sync.
349-359: No problems found with the alert logic here.
361-369: Event handler is consistent.
371-378: Error handling is straightforward here.
380-385: Push logic initialization looks fine.
386-387: Remove debug print statements.Consider removing
js/console.logto prevent noise in production logs.
388-405: Push request logic is correct.
407-413: Good handling of success event.
415-421: Remove the debug print at line 417.
js/console.erroris used for a server error message. Consider more structured logging or removing debug prints for production.
424-448: Polling logic is clearly implemented.
449-449: Removeconsole.warn.Debugging statements can clutter production logs.
450-453: No issues in the rest of the failure handler.
455-461: Reload event logic looks good.src/front/app/dashboard/subs.cljs (8)
2-2: No issues found in import changes.
53-56: Subscription registration is straightforward.
90-90: Changes here appear minimal.
95-98:require-git-repo?subscription is well-defined.
100-103: Subscription for repos looks fine.
105-108:is-sync-enabled?subscription logic is correct.
110-113: Loading state subscription is consistent.
115-122: Comparison logic formock-has-changes?is valid.src/back/api/ports/http_in.clj (4)
2-15: All good on the import statements.
No issues found; everything looks fine here, and no import reordering is required.
235-238: Use path manipulation functions for reliability.
A similar suggestion was previously made. Using Java’s path utilities (e.g.,java.io.File) or a library can robustly handle edge cases and enhance readability.
239-342: Consider concurrency/batching for large mock sets.
Prior feedback suggested parallel or batched processing to avoid performance bottlenecks when many mocks are updated.
357-357: Remove debug print statement.
Printing outsession-datain production code can be noisy. Use structured logging if you need visibility.src/front/app/dashboard/editor.cljs (4)
1-18: Inconsistent namespace usage remains partially unaddressed
This file still mixes thefront.app.dashboardnamespace with imports frommockingbird.lib. As noted previously, the namespace migration appears incomplete.
31-35: Well-structured polling lifecycle
This use-effect correctly starts and stops the mock polling when the component mounts or unmounts (or whenidchanges).
118-125: Reload button always showing spinner
As previously noted, the spinner is displayed unconditionally. Consider toggling it based on a loading state for clarity.
216-217: Check theitems-leftutility
Please confirmitems-leftis a valid custom Tailwind class or switch toitems-start.
| [{{{:keys [username install-id]} :body} :parameters | ||
| {:keys [user-id]} :session-data | ||
| {:keys [database]} :components | ||
| ctx :ctx}] | ||
| {:status 200 | ||
| :body {:user (controllers.user/edit-user! user-id username database ctx)}}) | ||
| :body {:user (controllers.user/edit-user! user-id username install-id database ctx)}}) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider validating or normalizing the install-id.
While adding install-id is a sensible extension, it might benefit from a simple validation (e.g., checking whether it’s numeric or not) before saving.
| [owner repo-name] (-> git-repo | ||
| (str/replace #"https://github.com/" "") | ||
| (str/split #"/")) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add robust validation when extracting owner and repo.
Splitting git-repo by slashes can fail if it does not include “github.com/owner/repo”. Consider safely validating the string structure or gracefully handling malformed URLs.
(let [git-user (controllers.user/get-user-by-id user-id components ctx)
- [owner repo-name] (-> git-repo
- (str/replace #"https://github.com/" "")
- (str/split #"/"))
+ [owner repo-name :as segments] (some-> git-repo
+ (str/replace #"^https://github.com/" "")
+ (str/split #"/"))
+ ;; Validate segments length before destructuring,
+ ;; e.g. (when (not= (count segments) 2) ...)
install-id (:git-install-id git-user)
...]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [owner repo-name] (-> git-repo | |
| (str/replace #"https://github.com/" "") | |
| (str/split #"/")) | |
| (let [git-user (controllers.user/get-user-by-id user-id components ctx) | |
| [owner repo-name :as segments] (some-> git-repo | |
| (str/replace #"^https://github.com/" "") | |
| (str/split #"/")) | |
| ;; Validate segments length before proceeding, e.g.: | |
| ;; (when (not= (count segments) 2) | |
| ;; (throw (ex-info "Invalid git-repo format" {:git-repo git-repo}))) | |
| install-id (:git-install-id git-user) | |
| ...] |
src/back/api/ports/http_in.clj
Outdated
| [{:keys [parameters session-data components ctx]}] | ||
| (let [user-id (:user-id session-data) | ||
| old-mock (:body parameters) | ||
| content (:content old-mock) | ||
| id (:id old-mock) | ||
| sha (:sha old-mock) | ||
| git-path (:git-repo old-mock) | ||
| git-repo (when git-path (last (str/split git-path #"/"))) | ||
| new-mock (controllers.mocks/update-mock! (parse-uuid (str id)) | ||
| (-> {} | ||
| (utils/assoc-if :content content) | ||
| (cond-> | ||
| (not= sha "") (utils/assoc-if :sha sha) | ||
| (not= git-repo "") (utils/assoc-if :git-repo git-path))) | ||
| components | ||
| ctx)] | ||
| {:status 200 | ||
| :body {:mock mock}})) | ||
| :body {:mock new-mock}})) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Refine checks for sha and git-repo.
Using (not= sha "") or (not= git-repo "") could inadvertently allow nil values. Consider (some? sha) or (not-empty sha) for clarity, ensuring only non-blank strings are used.
- (cond->
- (not= sha "") (utils/assoc-if :sha sha)
- (not= git-repo "") (utils/assoc-if :git-repo git-path))
+ (cond->
+ (not-empty sha) (utils/assoc-if :sha sha)
+ (not-empty git-repo) (utils/assoc-if :git-repo git-path))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [{:keys [parameters session-data components ctx]}] | |
| (let [user-id (:user-id session-data) | |
| old-mock (:body parameters) | |
| content (:content old-mock) | |
| id (:id old-mock) | |
| sha (:sha old-mock) | |
| git-path (:git-repo old-mock) | |
| git-repo (when git-path (last (str/split git-path #"/"))) | |
| new-mock (controllers.mocks/update-mock! (parse-uuid (str id)) | |
| (-> {} | |
| (utils/assoc-if :content content) | |
| (cond-> | |
| (not= sha "") (utils/assoc-if :sha sha) | |
| (not= git-repo "") (utils/assoc-if :git-repo git-path))) | |
| components | |
| ctx)] | |
| {:status 200 | |
| :body {:mock mock}})) | |
| :body {:mock new-mock}})) | |
| [{:keys [parameters session-data components ctx]}] | |
| (let [user-id (:user-id session-data) | |
| old-mock (:body parameters) | |
| content (:content old-mock) | |
| id (:id old-mock) | |
| sha (:sha old-mock) | |
| git-path (:git-repo old-mock) | |
| git-repo (when git-path (last (str/split git-path #"/"))) | |
| new-mock (controllers.mocks/update-mock! (parse-uuid (str id)) | |
| (-> {} | |
| (utils/assoc-if :content content) | |
| (cond-> | |
| (not-empty sha) (utils/assoc-if :sha sha) | |
| (not-empty git-repo) (utils/assoc-if :git-repo git-path))) | |
| components | |
| ctx)] | |
| {:status 200 | |
| :body {:mock new-mock}})) |
There was a problem hiding this comment.
@Felipe-gsilva acredito que isso faça sentido, não validar só ""
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@avelino realmente, eu nao coloquei a funcao q eu criei pra validar o sha ai 🫠
| (defnc editor-toolbar [{:keys [mock-id]}] | ||
| (let [has-changes? (refx/use-sub [:app.dashboard/mock-has-changes? mock-id]) | ||
| sync-enabled? (refx/use-sub [:app.dashboard/is-sync-enabled?])] | ||
| (d/div {:class "flex items-center space-x-2"} | ||
| (when-not sync-enabled? | ||
| ($ enable-sync-button {:mock-id mock-id})) | ||
| (when (and sync-enabled? has-changes?) | ||
| ($ reload-button {:mock-id mock-id})) | ||
| ($ remove-button {:mock-id mock-id}) | ||
| (if sync-enabled? | ||
| ($ save-button {:mock-id mock-id | ||
| :sync-enabled? sync-enabled? | ||
| :class "ml-auto "}) | ||
| ($ enable-sync-status {:mock-id mock-id | ||
| :sync-enabled? sync-enabled?}))))) |
There was a problem hiding this comment.
Conflicting enable-sync logic
• editor-toolbar calls enable-sync-button, yet actual sync enabling is done in enable-sync-status.
• The button dispatches a reload instead of enabling sync.
These duplicated or conflicting flows risk confusion. Unify your approach so the user’s click on “Enable Git Sync” actually enables sync.
+;; Option 1: Make the button directly enable sync, remove auto-enabling from `enable-sync-status`.
+;; Option 2: Remove the button if enabling is purely automatic.Committable suggestion skipped: line range outside the PR's diff.
| (defnc enable-sync-button [{:keys [mock-id]}] | ||
| (d/button | ||
| {:class (str "px-3 py-2 rounded-lg flex justify-center items-center space-x-2 " | ||
| "transition-all duration-75 ") | ||
| :on-click #(refx/dispatch-sync [:app.dashboard/reload-mock mock-id])} | ||
| (d/div {:class "text-black text-xs font-bold leading-[18px]"} | ||
| "enable git sync") | ||
| ($ loading-spinner))) | ||
|
|
There was a problem hiding this comment.
Mismatch between button label and dispatch action
Despite being called enable-sync-button, this component dispatches [:app.dashboard/reload-mock]. Update the dispatch action or rename the component to match its intent. Also consider conditionally showing the spinner.
Proposed fix
(defnc enable-sync-button [{:keys [mock-id]}]
(d/button
{:class (str "px-3 py-2 rounded-lg flex justify-center items-center space-x-2 "
"transition-all duration-75 ")
- :on-click #(refx/dispatch-sync [:app.dashboard/reload-mock mock-id])}
+ :on-click #(refx/dispatch-sync [:app.dashboard/enable-git-sync mock-id])}
(d/div {:class "text-black text-xs font-bold leading-[18px]"}
"enable git sync")
- ($ loading-spinner)))
+ ;; Optionally render the spinner only if enabling is actually in progress
+ ))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (defnc enable-sync-button [{:keys [mock-id]}] | |
| (d/button | |
| {:class (str "px-3 py-2 rounded-lg flex justify-center items-center space-x-2 " | |
| "transition-all duration-75 ") | |
| :on-click #(refx/dispatch-sync [:app.dashboard/reload-mock mock-id])} | |
| (d/div {:class "text-black text-xs font-bold leading-[18px]"} | |
| "enable git sync") | |
| ($ loading-spinner))) | |
| (defnc enable-sync-button [{:keys [mock-id]}] | |
| (d/button | |
| {:class (str "px-3 py-2 rounded-lg flex justify-center items-center space-x-2 " | |
| "transition-all duration-75 ") | |
| :on-click #(refx/dispatch-sync [:app.dashboard/enable-git-sync mock-id])} | |
| (d/div {:class "text-black text-xs font-bold leading-[18px]"} | |
| "enable git sync") | |
| ;; Optionally render the spinner only if enabling is actually in progress | |
| )) |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
resources/back/config.edn (1)
6-8:⚠️ Potential issueRemove hardcoded sensitive values from configuration.
Multiple sensitive configurations are exposed with potentially valid default values:
- Supabase API key and JWT secret
- AWS credentials
- Digital Ocean token
- Cloudflare token
- Database credentials
This poses a significant security risk if these values are valid or if they're accidentally used in production.
Recommendations:
- Remove all default values for sensitive configurations
- Use placeholder values (e.g., "REQUIRED") or throw errors when environment variables are missing
- Rotate any potentially valid tokens that have been exposed
- Consider using a secure secrets management solution
Example fix for one section:
-:supabase-api-key #or [#env SUPABASE_API_KEY "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJzdXBhYmFzZSIsInJlZiI6InRndmRmeHVyZ3NkZHhvdXhtdWdzIiwicm9sZSI6ImFub24iLCJpYXQiOjE2ODE1MDM0NDMsImV4cCI6MTk5NzA3OTQ0M30.7pq4MM_ZldiWvOk_cnQuxlvUF8eFcxlPDB7jMTNMYb0"] +:supabase-api-key #or [#env SUPABASE_API_KEY ""]Apply similar changes to all sensitive configurations.
Also applies to: 18-20, 34-34, 37-37, 44-44
🧰 Tools
🪛 Gitleaks (8.21.2)
6-6: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
resources/back/config.edn(1 hunks)src/front/app/dashboard/base.cljs(3 hunks)src/front/app/dashboard/editor.cljs(6 hunks)src/front/app/dashboard/events.cljs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: WIP
🔇 Additional comments (39)
resources/back/config.edn (1)
9-12: Add validation for required GitHub App credentials.While using empty defaults for sensitive values is good practice, the application should validate that required values are provided before attempting GitHub integration.
Consider adding runtime validation:
:git-sync {:secret #or [#env GITHUB_APP_SECRET ""] :app-id #or [#env GITHUB_APP_ID ""] :api-url #or [#env GITHUB_API_URL "https://api.github.com"] - :private-key #or [#env GITHUB_PRIVATE_KEY ""]} + :private-key #or [#env GITHUB_PRIVATE_KEY ""] + :validate-required? true}Then add validation logic in your initialization code to ensure these values are set when
:validate-required?is true.src/front/app/dashboard/base.cljs (7)
88-88: Use a mapping function instead ofdoseqto render DOM elements.
This is a repeat of a past review comment. Replacingdoseqwithmapormapvensures that each org element is returned and properly rendered in the DOM.
98-98: Revisit the switch fromdefnctodefn.
This repeats a prior concern that usingdefnin components leveraging hooks may cause issues. Consider reverting todefncunless there is a specific reason to avoid it.
150-150: Possible typo in class name'lm:col-span-3'.
This repeats a previous suggestion that'lm:col-span-3'might be a typo (e.g.,'sm:col-span-3') or an unintended breakpoint.
161-161:on-loadevent handler is not supported oninputelements.
Repeating a past comment: usehooks/use-effector a default value to initialize:wildcardinstead of:on-load.
176-176: Inconsistent usage of:classvs.:class-name.
This mirrors an earlier suggestion: replace:class-namewith:classto maintain consistency within Helix DOM elements.
292-319: Provide a fallback message when no repositories are available.
This is a repeat of a past comment: ifreposis empty, the modal remains blank. Consider rendering a short message like “No repositories found” for clarity.
321-321: Good usage ofdefncin a Helix component.
This usage helps ensure correct hook handling and naming resolution. No issues to report here.src/front/app/dashboard/editor.cljs (11)
1-12: All good here.
No major issues found in these lines; the namespace declaration and required libraries look appropriate.Also applies to: 14-18
13-13: Inconsistent namespace usage
31-35: Consider passing the mock ID to stop the polling
While the cleanup function calls[:app.dashboard/stop-mock-polling], it doesn’t specify themock-id. If the underlying logic supports multiple parallel polls, you might need to stop polling for the correct mock. Otherwise, verify that only one mock is polled at a time.Would you like me to generate a script to identify other occurrences of
stop-mock-pollingto confirm whether it needs themock-idargument?
77-81: LGTM: Drag-and-drop label
The drag-and-drop label, icon, and text are user-friendly and clear.
99-110: Add multi-file support if required
112-119: No issues found
The removal button logic is straightforward and consistent with typical re-frame patterns.
120-128: Unconditional spinner display
129-137: Mismatch between button label and dispatch action
152-166: Conflicting enable-sync logic
218-219: Check theitems-leftclass
243-258: LGTM: Top-level layout
These lines effectively combine the editor toolbar, file upload button, drag-drop interface, and editor in a cohesive UI.src/front/app/dashboard/events.cljs (20)
2-3: Imports look correct
Therefx.alphaimport is referenced properly. No issues found here.
24-25: Switch toreg-event-dbif only returning:db
This event returns only:dbwithout additional effects. Usingreg-event-dbwould simplify the handler.
26-31: Switch toreg-event-dbif only returning:db
This handler likewise returns only:db. Consider usingreg-event-dbfor consistency with recommended patterns.
39-45: Usereg-event-dbif no side effects
This handler updates only the database. Converting it toreg-event-dbclarifies that no other effects occur.
106-110: Headers and body construction look correct
No issues found; the construction of HTTP request headers and body is properly handled.
114-115: Appropriate use ofreg-event-fx
You dispatch a follow-up event and a user-facing notification, both of which are side effects, so usingreg-event-fxis correct.
160-161: LGTM
Changing:mocks-rawin the database after success is correctly handled here.
271-296: Remove debug print statements
(js/console.log username-to-save current-user)may leak user info and clutter production logs. Please remove or replace with a more secure logging approach.
297-301: Confirm naming consistency
Toggling the modal inverts:require-git-repo?. Verify that this accurately reflects the intended logic.
302-321: Remove debug prints
(js/console.log "Updating git repo with:" git-repo)is likely a leftover debug statement and should be removed for production.
322-331: Repository data handling is clear
Storing:repositoriesfrom the API response looks correct.
332-348: Usage ofreg-event-fxfor multiple side effects
Dispatching a check for missing repos and triggering the/synccall is well-organized.
361-370: Synchronization enabling logic looks correct
You handle success by toggling flags and showing a notification. No concerns here.
371-379: Error-handling approach is consistent
Properly sets:sync-enabledto false and notifies the user on sync failure.
380-406: Remove leftover debug logs
Lines 386 and 387 log the pushed mock details. These appear to be debug prints and should be removed or replaced with sanitized logs.
407-413: Clear success feedback
Marking:loading-pushfalse and showing a success notification is standard practice.
423-445: Periodic polling logic is well-structured
You schedule repeated API calls and handle success/failure outcomes. No immediate issues noted.
446-454: Remove debug statements or confirm necessity
js/console.warn "Poll failed:" ...might be needed for diagnosing errors, but it can also clutter logs.
455-462: Server mock update logic is clean
Conditionally updating:server-mockin the DB if data is present appears correct.
463-472: Reloading mock from server
Dispatching a partial edit request with server content, followed by a user-facing notification, looks well-defined.
| @@ -6,6 +6,10 @@ | |||
| :supabase-api-key #or [#env SUPABASE_API_KEY "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJzdXBhYmFzZSIsInJlZiI6InRndmRmeHVyZ3NkZHhvdXhtdWdzIiwicm9sZSI6ImFub24iLCJpYXQiOjE2ODE1MDM0NDMsImV4cCI6MTk5NzA3OTQ0M30.7pq4MM_ZldiWvOk_cnQuxlvUF8eFcxlPDB7jMTNMYb0"] | |||
| :supabase-url #or [#env SUPABASE_URL "https://tgvdfxurgsddxouxmugs.supabase.co"] | |||
| :supabase-jwt-secret #or [#env SUPABASE_JWT_SECRET "OP6cAnR4J7im0KH2sWeeTlwf4xQOZTuYtoO1mJ3vpON/szjCMpixe4r+9X3HvhZphxV79wl+2amIxo/AkfR28g=="] | |||
| :git-sync {:secret #or [#env GITHUB_APP_SECRET ""] | |||
| :app-id #or [#env GITHUB_APP_ID ""] | |||
| :api-url #or [#env GITHUB_API_URL "https://api.github.com"] | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make API URL environment-specific.
The GitHub API URL should be configurable per environment to support testing with GitHub Enterprise or mock servers in non-production environments.
- :api-url #or [#env GITHUB_API_URL "https://api.github.com"]
+ :api-url #profile {:dev #or [#env GITHUB_API_URL "http://localhost:8000"]
+ :prod #or [#env GITHUB_API_URL "https://api.github.com"]
+ :default #or [#env GITHUB_API_URL "https://api.github.com"]}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| :api-url #or [#env GITHUB_API_URL "https://api.github.com"] | |
| :api-url #profile {:dev #or [#env GITHUB_API_URL "http://localhost:8000"] | |
| :prod #or [#env GITHUB_API_URL "https://api.github.com"] | |
| :default #or [#env GITHUB_API_URL "https://api.github.com"]} |
| [helix.hooks :as hooks] | ||
| [refx.alpha :as refx])) | ||
|
|
||
| (defn settings-modal [] |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using defnc for Helix components using hooks.
Even though this function works with defn, it leverages Helix hooks (hooks/use-state), which typically requires Helix’s defnc macro to ensure proper hook usage and scope. Converting this to defnc can help prevent potential issues.
| (defnc enable-sync-status [{:keys [mock-id sync-enabled?]}] | ||
| (let [loading-sync? (refx/use-sub [:app.dashboard/loading-sync?])] | ||
| (hooks/use-effect | ||
| [] | ||
| (when-not sync-enabled? | ||
| (refx/dispatch-sync [:app.dashboard/enable-git-sync mock-id]))) | ||
|
|
||
| (when loading-sync? | ||
| (d/div | ||
| {:class "px-3 py-2 bg-gray-500 rounded-lg flex justify-center items-center space-x-2"} | ||
| (d/div {:class "text-white text-xs font-bold leading-[18px]"} | ||
| "Enabling sync...") | ||
| ($ loading-spinner))))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider re-running effect if sync state changes
The effect is only triggered once and won’t re-run if sync-enabled? toggles. If that’s intentional, keep as is; otherwise, consider adding sync-enabled? to the dependency vector.
| (defnc save-button [{:keys [mock-id full-width? sync-enabled?]}] | ||
| (let [mock-valid? (refx/use-sub [:app.dashboard/mock-valid?])] | ||
| (d/button | ||
| {:class-name (str "px-3 py-2 bg-pink-600 rounded-lg flex justify-center items-center btn-add space-x-2 " | ||
| (when full-width? "w-full ") | ||
| (when-not mock-valid? " opacity-50 cursor-not-allowed")) | ||
| {:class (str "w-full px-3 py-2 bg-pink-600 rounded-lg flex justify-center items-center btn-add space-x-2 transition-all duration-75 " | ||
| (when full-width? "w-full ") | ||
| (when-not mock-valid? " opacity-50 cursor-not-allowed")) | ||
| :on-click #(when mock-valid? | ||
| (refx/dispatch-sync [:app.dashboard/save-mock mock-id]) | ||
| (refx/dispatch-sync [:app.dashboard/push-mock mock-id]) | ||
| (refx/dispatch-sync [:app.dashboard/reset-mock-pub-stts mock-id]))} | ||
| (d/div {:class-name "text-white text-xs font-bold leading-[18px]"} " save") | ||
| ($ svg/save)))) | ||
| (d/div {:class "text-white text-xs font-bold leading-[18px]"} "save") | ||
| (if sync-enabled? | ||
| ($ svg/github) | ||
| ($ svg/save))))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Separate push action from save if needed
Currently “save” also triggers push-mock. This may surprise users who want to save local changes without pushing. If that’s desirable, great. Otherwise, consider splitting “save” and “push” to give finer control.
:on-click #(when mock-valid?
(refx/dispatch-sync [:app.dashboard/save-mock mock-id])
- (refx/dispatch-sync [:app.dashboard/push-mock mock-id])
(refx/dispatch-sync [:app.dashboard/reset-mock-pub-stts mock-id]))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (defnc save-button [{:keys [mock-id full-width? sync-enabled?]}] | |
| (let [mock-valid? (refx/use-sub [:app.dashboard/mock-valid?])] | |
| (d/button | |
| {:class-name (str "px-3 py-2 bg-pink-600 rounded-lg flex justify-center items-center btn-add space-x-2 " | |
| (when full-width? "w-full ") | |
| (when-not mock-valid? " opacity-50 cursor-not-allowed")) | |
| {:class (str "w-full px-3 py-2 bg-pink-600 rounded-lg flex justify-center items-center btn-add space-x-2 transition-all duration-75 " | |
| (when full-width? "w-full ") | |
| (when-not mock-valid? " opacity-50 cursor-not-allowed")) | |
| :on-click #(when mock-valid? | |
| (refx/dispatch-sync [:app.dashboard/save-mock mock-id]) | |
| (refx/dispatch-sync [:app.dashboard/push-mock mock-id]) | |
| (refx/dispatch-sync [:app.dashboard/reset-mock-pub-stts mock-id]))} | |
| (d/div {:class-name "text-white text-xs font-bold leading-[18px]"} " save") | |
| ($ svg/save)))) | |
| (d/div {:class "text-white text-xs font-bold leading-[18px]"} "save") | |
| (if sync-enabled? | |
| ($ svg/github) | |
| ($ svg/save))))) | |
| (defnc save-button [{:keys [mock-id full-width? sync-enabled?]}] | |
| (let [mock-valid? (refx/use-sub [:app.dashboard/mock-valid?])] | |
| (d/button | |
| {:class (str "w-full px-3 py-2 bg-pink-600 rounded-lg flex justify-center items-center btn-add space-x-2 transition-all duration-75 " | |
| (when full-width? "w-full ") | |
| (when-not mock-valid? " opacity-50 cursor-not-allowed")) | |
| :on-click #(when mock-valid? | |
| (refx/dispatch-sync [:app.dashboard/save-mock mock-id]) | |
| (refx/dispatch-sync [:app.dashboard/reset-mock-pub-stts mock-id]))} | |
| (d/div {:class "text-white text-xs font-bold leading-[18px]"} "save") | |
| (if sync-enabled? | |
| ($ svg/github) | |
| ($ svg/save))))) |
| (let [mock-id (:id data) | ||
| has-changes? (refx/use-sub [:app.dashboard/mock-has-changes? mock-id])] | ||
| (d/div {:class (str "relative w-full h-full rounded-lg overflow-hidden m-2 " | ||
| (if has-changes? "border-2 border-yellow-500 " | ||
| "border-none " | ||
| ))} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove the extra trailing space in the border style
The if has an additional space after "border-none ". While harmless, it slightly clutters the string construction. Consider removing it for consistency.
- (if has-changes? "border-2 border-yellow-500 " "border-none ")
+ (if has-changes? "border-2 border-yellow-500 " "border-none")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (let [mock-id (:id data) | |
| has-changes? (refx/use-sub [:app.dashboard/mock-has-changes? mock-id])] | |
| (d/div {:class (str "relative w-full h-full rounded-lg overflow-hidden m-2 " | |
| (if has-changes? "border-2 border-yellow-500 " | |
| "border-none " | |
| ))} | |
| (let [mock-id (:id data) | |
| has-changes? (refx/use-sub [:app.dashboard/mock-has-changes? mock-id])] | |
| (d/div {:class (str "relative w-full h-full rounded-lg overflow-hidden m-2 " | |
| (if has-changes? "border-2 border-yellow-500 " "border-none"))} |
| (fn [{db :db} [_ response]] | ||
| (js/console.error "Save mock failed:", (clj->js response)) | ||
| {:db (assoc db | ||
| :loading-edit-mock false | ||
| :loading-sync? false) | ||
| :notification {:type :error | ||
| :content (or (-> response :body :error :message) | ||
| "Failed to save mock")}})) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure no sensitive data is logged
Logging errors is useful, but verify that response does not contain tokens or PII to avoid console exposure.
| (refx/reg-event-fx | ||
| :app.dashboard/verify-mock-repo | ||
| (fn [db [_ mock-id]] | ||
| {:notification {:type :error | ||
| :content "Mock has no linked repo"} | ||
| :http {:url "/repos" | ||
| :method :get | ||
| :headers {"authorization" (str "Bearer " (-> db :current-user :access-token))} | ||
| :on-success [:app.dashboard/save-repos] | ||
| :on-failure [:app.dashboard/save-mock-failed]} | ||
| :db (assoc db :curr-mock-id mock-id)})) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Inconsistent destructuring for reg-event-fx
Currently (fn [db [_ mock-id]] ...) is used, but other handlers destructure {db :db}. Consider adopting a consistent pattern for clarity.
| (refx/reg-event-fx | ||
| :app.dashboard/push-mock-failed | ||
| (fn [{db :db} [_ response]] | ||
| (js/console.error "Push failed:", (clj->js response)) | ||
| {:db (assoc db :loading-push false) | ||
| :notification {:type :error | ||
| :content (or (-> response :body :message) | ||
| "Failed to push to GitHub")}})) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure no sensitive info is printed
(js/console.error "Push failed:", ...) helps debugging, but confirm that it doesn’t expose tokens or PII in production logs.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/back/api/ports/http_in.clj(8 hunks)src/back/api/utils.clj(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/back/api/ports/http_in.clj (1)
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#375
File: src/back/api/ports/http_in.clj:218-277
Timestamp: 2025-01-24T13:37:14.472Z
Learning: The webhook endpoint in moclojer-app verifies GitHub webhook signatures using the verify-webhook-signature interceptor from back.api.interceptors.verify-webhook, which uses clj-github-app.webhook-signature library.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: WIP
🔇 Additional comments (12)
src/back/api/utils.clj (5)
1-3: LGTM!The namespace declaration and imports are appropriate for the functionality provided.
5-11: Enhance docstring clarity.The docstring could be more specific about what constitutes a valid value (truthy vs. falsy in Clojure).
-(defn assoc-if - "This function returns a map with a respective associated key and value - if it is given as argument." - [m k v] +(defn assoc-if + "Associates key k with value v in map m, only if v is not nil or false. + Returns the original map unchanged if v is nil or false." + [m k v]
13-18: Add input validation and error handling for decode function.The function should handle edge cases and malformed input safely, especially important for webhook payloads.
(defn decode "Decodes a given string from base64 format" [s] - (let [cleaned-str (str/replace s #"\s+" "") - decoder (java.util.Base64/getDecoder)] - (String. (.decode decoder cleaned-str) "UTF-8"))) + (when (string? s) + (try + (let [cleaned-str (str/replace s #"\s+" "") + decoder (java.util.Base64/getDecoder)] + (String. (.decode decoder cleaned-str) "UTF-8")) + (catch IllegalArgumentException e + (throw (ex-info "Invalid base64 input" + {:type :validation-error + :cause (.getMessage e)}))) + (catch Exception e + (throw (ex-info "Failed to decode base64" + {:type :decode-error + :cause (.getMessage e)}))))))
20-24: Add input validation and error handling for encode function.Similar to decode, the function should handle edge cases safely.
(defn encode "Encodes a given string into base64 format" [s] - (let [encoder (java.util.Base64/getEncoder)] - (.encodeToString encoder (.getBytes s "UTF-8")))) + (when (string? s) + (try + (let [encoder (java.util.Base64/getEncoder)] + (.encodeToString encoder (.getBytes s "UTF-8"))) + (catch Exception e + (throw (ex-info "Failed to encode to base64" + {:type :encode-error + :cause (.getMessage e)}))))))
26-31: Optimize validation order and enhance docstring.The validation checks can be reordered for better performance, and the docstring could be more descriptive.
(defn sha256? - "Checks if the input string is a valid SHA-256 hash." + "Validates if the input is a valid SHA-256 hash string. + Returns true if the input is a 64-character string containing only hexadecimal digits (0-9, a-f, A-F)." [s] - (and (string? s) - (= 64 (count s)) - (re-matches #"^[0-9a-fA-F]{64}$" s))) + (and (= 64 (count s)) ; fastest check first + (string? s) ; type check second + (re-matches #"^[0-9a-fA-F]{64}$" s))) ; regex lastsrc/back/api/ports/http_in.clj (7)
2-15: All set with the new require statements.
These additions cleanly integrate the controllers and logic namespaces without obvious duplication or circular references.
26-31: No concerns with the extended parameters.
Allowinginstall-idinedit-user!appears consistent with broader sync logic.
45-45: Good retrieval approach for user data.
Passing{:database database}toget-user-by-idmatches existing patterns.
66-68: Expanded function signature is cohesive.
Extracting parameters, session-data, and components in one map ensures easier maintenance.
235-238: Filtering approach is straightforward.
Thefilter-mockshelper is clear and helps isolate the logic for locatingmoclojer.ymlfiles.
355-358: Remove debug print statement in production code.
Printing session data to stdout may leak sensitive data. This mirrors a previous comment about removing debug statements.
375-377: Robust parsing needed for owner/repo extraction.
When splittinggit-repo, handle edge cases (extra slashes, missing scheme, trailing slash) to avoid partial or malformed splits. This duplicates earlier suggestions about making path parsing more reliable.
| (defn github-link? | ||
| "Checks if the input string correctly represents a github repo link" | ||
| [s] | ||
| (and (string? s) | ||
| (not-empty s) | ||
| (str/starts-with? s "https://github.com/"))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance GitHub repository link validation.
The current validation is basic and might miss edge cases. Consider using a more specific regex pattern to validate repository names.
(defn github-link?
"Checks if the input string correctly represents a github repo link"
[s]
(and (string? s)
(not-empty s)
- (str/starts-with? s "https://github.com/")))
+ (re-matches #"^https://github\.com/[a-zA-Z0-9](?:[a-zA-Z0-9]|-(?=[a-zA-Z0-9])){0,38}/[a-zA-Z0-9_.-]+$" s)))This regex pattern ensures:
- Valid GitHub username format (39 chars max, alphanumeric with single hyphens between)
- Valid repository name format (alphanumeric with underscores, dots, and hyphens)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (defn github-link? | |
| "Checks if the input string correctly represents a github repo link" | |
| [s] | |
| (and (string? s) | |
| (not-empty s) | |
| (str/starts-with? s "https://github.com/"))) | |
| (defn github-link? | |
| "Checks if the input string correctly represents a github repo link" | |
| [s] | |
| (and (string? s) | |
| (not-empty s) | |
| (re-matches #"^https://github\.com/[a-zA-Z0-9](?:[a-zA-Z0-9]|-(?=[a-zA-Z0-9])){0,38}/[a-zA-Z0-9_.-]+$" s))) |
| (defn handler-get-mock-by-id | ||
| [{:keys [parameters session-data components ctx]}] | ||
| (let [id (-> parameters :path :id) | ||
| user (controllers.user/get-user-by-id | ||
| (:user-id session-data) components ctx) | ||
| mocks (controllers.mocks/get-mocks user components ctx) | ||
| mock (->> mocks | ||
| (mapcat :apis) | ||
| (filter #(= (str (:id %)) id)) | ||
| first)] | ||
| (if mock | ||
| {:status 200 | ||
| :body {:mock mock}} | ||
| {:status 404 | ||
| :body {:error "Mock not found"}}))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Potential performance concern for large datasets.
Collecting all mocks and filtering in memory (mapcat :apis) may become costly if the user has a large number of mocks. If performance is impacted, consider an indexed or direct query approach.
| install-id (get-in body [:installation :id])] | ||
| (logs/log :info "Webhook received" | ||
| :ctx (assoc ctx :event-type event-type)) | ||
| (cond | ||
| (nil? install-id) | ||
| {:status 401 | ||
| :body {:message "Forbidden"}} | ||
| (= event-type "push") | ||
| (let [repo (:repository body) | ||
| commits (:commits body) | ||
| git-slug (get-in repo [:owner :name]) | ||
| repo-name (:name repo) | ||
| org (controllers.orgs/get-by-git-orgname git-slug components ctx) | ||
| user (controllers.user/get-by-git-username git-slug components ctx) | ||
| id (or (parse-uuid (:id org)) (parse-uuid (:uuid user))) | ||
| slug (or (:orgname org) (:username user))] | ||
| (logs/log :info "Processing Push Event" | ||
| :ctx (assoc ctx | ||
| :id id | ||
| :repo repo-name)) | ||
|
|
||
| (if-not id | ||
| {:status 404 | ||
| :body {:message "User and org not found"}} | ||
| (let [user-mocks (controllers.mocks/get-mocks {:uuid id :username slug} components ctx) | ||
| mocks (filter-mocks (->> commits | ||
| (mapcat #(vals (select-keys % [:added :modified]))) | ||
| (apply concat) | ||
| vec)) | ||
| source-files (when mocks (controllers.sync/pull! install-id git-slug repo-name mocks components ctx))] | ||
| (when (seq source-files) | ||
| (logs/log :info "Processing modified mocks" | ||
| :ctx (assoc ctx | ||
| :mocks mocks | ||
| :user-mocks user-mocks | ||
| :modified-count (count source-files))) | ||
| (doseq [mock source-files | ||
| :let [content (utils/decode (get-in mock [:content :content])) | ||
| sha (get-in mock [:content :sha]) | ||
| ;; "resources/mocks/moclojer-test/moclojer.yml" ==> moclojer-test | ||
| wildcard (-> (:file mock) | ||
| (str/split #"/") | ||
| (as-> e (take-last 2 e)) | ||
| (first)) | ||
| existing-mock (when user-mocks | ||
| (first (filter #(= (:wildcard %) wildcard) | ||
| (->> user-mocks | ||
| first | ||
| :apis | ||
| (map #(select-keys % [:wildcard :id]))))))]] | ||
| (if (seq existing-mock) | ||
| (if-let [mock-id (:id existing-mock)] | ||
| (try | ||
| (controllers.mocks/update-mock! mock-id | ||
| {:content content | ||
| :git-repo (:url repo) | ||
| :sha sha} | ||
| components ctx) | ||
| (catch Exception e | ||
| (logs/log :error "Failed to update mock" | ||
| :ctx (assoc ctx :error (.getMessage e) :mock-id mock-id)) | ||
| {:status 500 | ||
| :body {:message (.getMessage e)}})) | ||
| (logs/log :error "Mock found but has no ID" | ||
| :ctx (assoc ctx :wildcard wildcard))) | ||
| (try | ||
| (controllers.mocks/create-mock! | ||
| id | ||
| (logic.mocks/create {:content content | ||
| :sha sha | ||
| :wildcard wildcard | ||
| :subdomain slug | ||
| :enabled true | ||
| :git-repo (:url repo)}) | ||
| components ctx) | ||
| (catch Exception e | ||
| (logs/log :error "Failed to create mock" | ||
| :ctx (assoc ctx :error (.getMessage e))) | ||
| {:status 500 | ||
| :body {:message (.getMessage e)}}))))) | ||
| {:status 200 | ||
| :body {:message "Files updated from source"}}))) | ||
| (#{"installation" "installation_repositories"} event-type) | ||
| (let [sender-type (get-in body [:installation :account :type]) | ||
| git-slug (get-in body [:installation :account :login]) | ||
| org (when (= sender-type "Organization") (controllers.orgs/get-by-git-orgname git-slug components ctx)) | ||
| user (when (= sender-type "User") (controllers.user/get-by-git-username git-slug components ctx)) | ||
| id (or (parse-uuid (:id org)) (parse-uuid (:uuid user)))] | ||
| (logs/log :info "Processing Installation Event" | ||
| :ctx (assoc ctx :id id)) | ||
| (if-not (nil? (or (:id org) (:uuid user))) | ||
| {:status 200 | ||
| :body {:message "Enabled Git Sync" | ||
| :content (if org | ||
| (controllers.orgs/enable-sync install-id id components ctx) | ||
| (controllers.user/enable-sync install-id id components ctx))}} | ||
| {:status 404 | ||
| :body {:message "no org or user found"}})) | ||
| :else {:status 400 | ||
| :body {:message "Unhandled event type"}}))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider extracting logic from this large handler-webhook for maintainability.
Processing installation events, push events, and file updates in one function can be broken down into smaller, focused functions (e.g., handle-push-event, handle-installation-event). This reduces complexity and clarifies the flow.
| git-path (:git-repo old-mock) | ||
| git-repo (when git-path (last (str/split git-path #"/"))) | ||
| new-mock (controllers.mocks/update-mock! (parse-uuid (str id)) | ||
| (-> {} | ||
| (utils/assoc-if :content content) | ||
| (cond-> | ||
| (utils/sha256? sha) (utils/assoc-if :sha sha) | ||
| (utils/github-link? git-repo) (utils/assoc-if :git-repo git-path))) | ||
| components | ||
| ctx)] | ||
| {:status 200 |
There was a problem hiding this comment.
Potential mismatch in GitHub link check and missing UUID error handling.
- You are only retaining the last path segment in
git-repo(line 74) but using(utils/github-link? git-repo)for the conditional check (line 80). This likely yieldsfalsefor valid GitHub URLs sincegit-repois no longer the full link. Instead, consider:
- git-repo (when git-path (last (str/split git-path #"/")))
- ...
- (cond->
- (utils/sha256? sha) (utils/assoc-if :sha sha)
- (utils/github-link? git-repo) (utils/assoc-if :git-repo git-path))
+ repo-name (when git-path (last (str/split git-path #"/")))
+ ...
+ (cond->
+ (utils/sha256? sha) (utils/assoc-if :sha sha)
+ (utils/github-link? git-path) (utils/assoc-if :git-repo git-path))(parse-uuid (str id))can throw an exception ifidis invalid. Consider safe parsing or returning a clear error.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| git-path (:git-repo old-mock) | |
| git-repo (when git-path (last (str/split git-path #"/"))) | |
| new-mock (controllers.mocks/update-mock! (parse-uuid (str id)) | |
| (-> {} | |
| (utils/assoc-if :content content) | |
| (cond-> | |
| (utils/sha256? sha) (utils/assoc-if :sha sha) | |
| (utils/github-link? git-repo) (utils/assoc-if :git-repo git-path))) | |
| components | |
| ctx)] | |
| {:status 200 | |
| git-path (:git-repo old-mock) | |
| repo-name (when git-path (last (str/split git-path #"/"))) | |
| new-mock (controllers.mocks/update-mock! (parse-uuid (str id)) | |
| (-> {} | |
| (utils/assoc-if :content content) | |
| (cond-> | |
| (utils/sha256? sha) (utils/assoc-if :sha sha) | |
| (utils/github-link? git-path) (utils/assoc-if :git-repo git-path))) | |
| components | |
| ctx)] | |
| {:status 200 |
fixes #257 #372 #366
Summary by CodeRabbit
Summary by CodeRabbit
New Features
/webhookendpoint.git_install_idfield toorgandcustomertables for improved tracking.git_install_id,git_repo, andsha.settings-modalfor user settings management.Bug Fixes
Improvements
Refactor
:class-nameto:classacross components.Chores