Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded GitHub account-link management: the Account settings UI now checks link existence, shows loading/stateful Enable/Disable controls, and redirects to a service-provided authorization URL to initiate linking; the auth service client gained methods to init and delete GitHub links. Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/app/component/settings/Account.tsx`:
- Around line 93-96: The current createResource for githubLinkExists masks API
failures by returning false when response is missing; change the async loader
(the function passed to createResource that calls
authServiceClient.checkLinkExists) to stop defaulting to false and instead
propagate errors or return undefined/null so the resource can enter an error or
loading state (e.g., return response?.link_exists without the ?? false, or
rethrow when the RPC fails). Update any UI that consumes githubLinkExists (and
refetchGithubLink) to handle the error/undefined state instead of treating it as
"not linked."
- Around line 93-108: The component currently calls authServiceClient directly
(createResource/githubLinkExists, refetchGithubLink, handleGithubEnable calling
authServiceClient.initGithubLink, and handleGithubDisable calling
authServiceClient.deleteGithubLink); move these network calls into the queries
package by creating a TanStack Query read hook for checking GitHub link
existence (e.g., useGithubLinkExistsQuery) and mutation hooks for initGithubLink
and deleteGithubLink (e.g., useInitGithubLinkMutation,
useDeleteGithubLinkMutation), then replace the createResource and direct client
calls in Account.tsx with those hooks and call the mutations (and
invalidate/refetch the existence query) instead of using authServiceClient
directly.
- Line 101: The current window.open call (in Account.tsx) opens a URL with
target '_blank' allowing tabnabbing; update the open to use a safe pattern by
passing noopener,noreferrer in the features string or by capturing the returned
window object and setting its opener to null (e.g., use window.open(url,
'_blank', 'noopener,noreferrer') or const w = window.open(...); if (w) w.opener
= null) so the opened page cannot access window.opener.
In `@js/app/packages/service-clients/service-auth/client.ts`:
- Around line 471-477: The deleteGithubLink function currently calls
fetchWithAuth with a generic of {} and uses a no-op block mapper; change the
generic to use the imported EmptyResponse type and replace the mapper with an
explicit () => undefined so the return type is tightened and clearer; update the
call in deleteGithubLink (and its mapOk invocation) to use
fetchWithAuth<EmptyResponse>(...) and mapOk(..., () => undefined).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34513de8-19cd-42b0-8e18-fd2f8eef7c6f
📒 Files selected for processing (2)
js/app/packages/app/component/settings/Account.tsxjs/app/packages/service-clients/service-auth/client.ts
Comment on lines
+93
to
+108
| const [githubLinkExists, { refetch: refetchGithubLink }] = createResource(async () => { | ||
| const [_, response] = await authServiceClient.checkLinkExists({ idp_name: 'github' }); | ||
| return response?.link_exists ?? false; | ||
| }); | ||
|
|
||
| const handleGithubEnable = async () => { | ||
| const [_, url] = await authServiceClient.initGithubLink(); | ||
| if (url) { | ||
| window.open(url, '_blank'); | ||
| } | ||
| }; | ||
|
|
||
| const handleGithubDisable = async () => { | ||
| await authServiceClient.deleteGithubLink(); | ||
| refetchGithubLink(); | ||
| }; |
Contributor
There was a problem hiding this comment.
Move GitHub link API calls out of the component and into queries.
Lines 93-108 call authServiceClient directly from a TSX component. This bypasses the required TanStack Query boundary and should be moved to query/mutation hooks in the queries package.
As per coding guidelines, "All network calls to service clients MUST go through TanStack Query in the queries package. Do NOT call service clients directly from components or other packages."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/settings/Account.tsx` around lines 93 - 108,
The component currently calls authServiceClient directly
(createResource/githubLinkExists, refetchGithubLink, handleGithubEnable calling
authServiceClient.initGithubLink, and handleGithubDisable calling
authServiceClient.deleteGithubLink); move these network calls into the queries
package by creating a TanStack Query read hook for checking GitHub link
existence (e.g., useGithubLinkExistsQuery) and mutation hooks for initGithubLink
and deleteGithubLink (e.g., useInitGithubLinkMutation,
useDeleteGithubLinkMutation), then replace the createResource and direct client
calls in Account.tsx with those hooks and call the mutations (and
invalidate/refetch the existence query) instead of using authServiceClient
directly.
| const handleGithubEnable = async () => { | ||
| const [_, url] = await authServiceClient.initGithubLink(); | ||
| if (url) { | ||
| window.open(url, '_blank'); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "Account.tsx" | grep -E "js/app/packages/app/component/settings"Repository: macro-inc/macro
Length of output: 111
🏁 Script executed:
cat -n ./js/app/packages/app/component/settings/Account.tsx | sed -n '95,110p'Repository: macro-inc/macro
Length of output: 538
🏁 Script executed:
# Check for other window.open calls in the same file
rg "window\.open" ./js/app/packages/app/component/settings/Account.tsx -nRepository: macro-inc/macro
Length of output: 96
🏁 Script executed:
# Check if there are other similar patterns in the js/app directory
rg "window\.open\([^,]+,\s*['\"]_blank['\"](?!\s*,\s*['\"].*(?:noopener|noreferrer))" ./js/app --type tsx --type ts -A 1 | head -50Repository: macro-inc/macro
Length of output: 376
🏁 Script executed:
# Search for all window.open calls in js/app
rg "window\.open\(" ./js/app --type tsx --type ts -B 2 -A 1Repository: macro-inc/macro
Length of output: 86
🏁 Script executed:
# Search for all window.open calls in js/app
rg "window\.open\(" ./js/app -B 2 -A 1Repository: macro-inc/macro
Length of output: 4185
Harden the new-tab redirect against tabnabbing.
Line 101 uses window.open(url, '_blank') without noopener,noreferrer, which allows the opened page to access window.opener.
🔒 Suggested fix
- window.open(url, '_blank');
+ window.open(url, '_blank', 'noopener,noreferrer');📝 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.
Suggested change
| window.open(url, '_blank'); | |
| window.open(url, '_blank', 'noopener,noreferrer'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/settings/Account.tsx` at line 101, The current
window.open call (in Account.tsx) opens a URL with target '_blank' allowing
tabnabbing; update the open to use a safe pattern by passing noopener,noreferrer
in the features string or by capturing the returned window object and setting
its opener to null (e.g., use window.open(url, '_blank', 'noopener,noreferrer')
or const w = window.open(...); if (w) w.opener = null) so the opened page cannot
access window.opener.
Comment on lines
+471
to
+477
| async deleteGithubLink() { | ||
| return mapOk( | ||
| await fetchWithAuth<{}>(`${authHost}/link/github`, { | ||
| method: 'DELETE', | ||
| }), | ||
| (_result) => {} | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tighten delete-link response typing for clarity.
Use EmptyResponse (already imported) and an explicit () => undefined mapper instead of {} + empty block return.
♻️ Suggested refactor
async deleteGithubLink() {
return mapOk(
- await fetchWithAuth<{}>(`${authHost}/link/github`, {
+ await fetchWithAuth<EmptyResponse>(`${authHost}/link/github`, {
method: 'DELETE',
}),
- (_result) => {}
+ () => undefined
);
},📝 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.
Suggested change
| async deleteGithubLink() { | |
| return mapOk( | |
| await fetchWithAuth<{}>(`${authHost}/link/github`, { | |
| method: 'DELETE', | |
| }), | |
| (_result) => {} | |
| ); | |
| async deleteGithubLink() { | |
| return mapOk( | |
| await fetchWithAuth<EmptyResponse>(`${authHost}/link/github`, { | |
| method: 'DELETE', | |
| }), | |
| () => undefined | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/service-clients/service-auth/client.ts` around lines 471 -
477, The deleteGithubLink function currently calls fetchWithAuth with a generic
of {} and uses a no-op block mapper; change the generic to use the imported
EmptyResponse type and replace the mapper with an explicit () => undefined so
the return type is tightened and clearer; update the call in deleteGithubLink
(and its mapOk invocation) to use fetchWithAuth<EmptyResponse>(...) and
mapOk(..., () => undefined).
whutchinson98
force-pushed
the
hutch/macro-cyp4cgVvbdJJHQxGHGHei-connect-github-button-integration
branch
from
4a6c392 to
aa6755d
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
js/app/packages/service-clients/service-auth/client.ts (1)
474-480: 🧹 Nitpick | 🔵 TrivialTighten delete-link response typing and mapper.
Use
EmptyResponse+() => undefinedinstead of<{}>+ empty block mapper for a clearer return contract.♻️ Proposed refactor
async deleteGithubLink() { return mapOk( - await fetchWithAuth<{}>(`${authHost}/link/github`, { + await fetchWithAuth<EmptyResponse>(`${authHost}/link/github`, { method: 'DELETE', }), - (_result) => {} + () => undefined ); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/service-clients/service-auth/client.ts` around lines 474 - 480, The deleteGithubLink implementation should tighten its response typing and mapper: in deleteGithubLink replace the fetchWithAuth generic from <{}> to <EmptyResponse> and change the mapOk mapper from (_result) => {} to () => undefined so the function clearly returns void/undefined; also ensure EmptyResponse is imported or available in this module and update any surrounding types accordingly (symbols: deleteGithubLink, fetchWithAuth, mapOk, EmptyResponse).js/app/packages/app/component/settings/Account.tsx (2)
93-96:⚠️ Potential issue | 🟡 MinorDo not coerce failed GitHub link checks to
false.Line 95 maps missing/error responses to “not linked,” which can show the wrong action. Preserve error/undefined so UI can render loading/error distinctly.
💡 Minimal fix in current pattern
const [githubLinkExists, { refetch: refetchGithubLink }] = createResource(async () => { - const [_, response] = await authServiceClient.checkLinkExists({ idp_name: 'github' }); - return response?.link_exists ?? false; + const [error, response] = await authServiceClient.checkLinkExists({ idp_name: 'github' }); + if (error) throw new Error(error[0]?.message ?? 'Failed to check GitHub link'); + return response?.link_exists; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/component/settings/Account.tsx` around lines 93 - 96, The current createResource for githubLinkExists coerces missing or errored responses to false (returning response?.link_exists ?? false), which hides errors from the UI; change the handler in the createResource used for githubLinkExists (and keep refetchGithubLink) to return response?.link_exists without the "?? false" so the value can be boolean | undefined, and avoid swallowing exceptions (let errors propagate or rethrow instead of converting them to false) so the UI can render loading/error states distinctly.
93-108:⚠️ Potential issue | 🟠 MajorMove GitHub link network calls into
queries(TanStack Query boundary).
createResource+ directauthServiceClientusage here bypasses the required data-access layer. Please expose query/mutation hooks fromqueriesand consume those in this component.As per coding guidelines, "All network calls to service clients MUST go through TanStack Query in the
queriespackage. Do NOT call service clients directly from components or other packages."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/component/settings/Account.tsx` around lines 93 - 108, The component currently uses createResource and calls authServiceClient directly (see githubLinkExists + refetchGithubLink, handleGithubEnable, handleGithubDisable), which bypasses the TanStack Query layer; move these network operations into the queries package by adding a query hook (e.g., useGithubLinkExistsQuery) for checkLinkExists and mutation hooks (e.g., useInitGithubLinkMutation, useDeleteGithubLinkMutation) for initGithubLink and deleteGithubLink, then replace createResource and direct calls in this component with the new hooks (subscribe to the query for link state and call the mutations in handleGithubEnable/handleGithubDisable, invoking refetch or relying on mutation invalidate logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@js/app/packages/app/component/settings/Account.tsx`:
- Around line 93-96: The current createResource for githubLinkExists coerces
missing or errored responses to false (returning response?.link_exists ??
false), which hides errors from the UI; change the handler in the createResource
used for githubLinkExists (and keep refetchGithubLink) to return
response?.link_exists without the "?? false" so the value can be boolean |
undefined, and avoid swallowing exceptions (let errors propagate or rethrow
instead of converting them to false) so the UI can render loading/error states
distinctly.
- Around line 93-108: The component currently uses createResource and calls
authServiceClient directly (see githubLinkExists + refetchGithubLink,
handleGithubEnable, handleGithubDisable), which bypasses the TanStack Query
layer; move these network operations into the queries package by adding a query
hook (e.g., useGithubLinkExistsQuery) for checkLinkExists and mutation hooks
(e.g., useInitGithubLinkMutation, useDeleteGithubLinkMutation) for
initGithubLink and deleteGithubLink, then replace createResource and direct
calls in this component with the new hooks (subscribe to the query for link
state and call the mutations in handleGithubEnable/handleGithubDisable, invoking
refetch or relying on mutation invalidate logic).
In `@js/app/packages/service-clients/service-auth/client.ts`:
- Around line 474-480: The deleteGithubLink implementation should tighten its
response typing and mapper: in deleteGithubLink replace the fetchWithAuth
generic from <{}> to <EmptyResponse> and change the mapOk mapper from (_result)
=> {} to () => undefined so the function clearly returns void/undefined; also
ensure EmptyResponse is imported or available in this module and update any
surrounding types accordingly (symbols: deleteGithubLink, fetchWithAuth, mapOk,
EmptyResponse).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ae60df3-d0fb-47bc-be00-d84a1b60f9ef
📒 Files selected for processing (2)
js/app/packages/app/component/settings/Account.tsxjs/app/packages/service-clients/service-auth/client.ts
whutchinson98
deleted the
hutch/macro-cyp4cgVvbdJJHQxGHGHei-connect-github-button-integration
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.