refactor: derive the current branch from the URL instead of syncing an atom - #10128
Conversation
…n atom The branch query string parameter and `currentBranchAtom` held the same state, kept in step by an effect in `BranchesProvider`. `currentBranch` is now derived during render, the atom is deleted, and `setCurrentBranch` writes the query string parameter. Removing the sync also removes the render guard that compared the resolved branch against the hardcoded `main`, which left the app on a permanent loading screen when `INFRAHUB_INITIAL_DEFAULT_BRANCH` was set to another name. `isFieldDisabled` no longer reads the atom through the global Jotai store: it takes `isDefaultBranch`, threaded from the form call sites, and is a pure function again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
947d509 to
f2e40fe
Compare
There was a problem hiding this comment.
Review completed against the latest diff
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
The fixture default branch is named `primary` rather than `main`, so the tests fail against the removed guard that compared the resolved branch against the hardcoded default branch name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Derive the query mock state from the hook's return type instead of a hand-written shape that could drift from it, and cover two more cases: an explicit default branch in the URL, and a background refetch that must not replace the children with the loading screen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Confidence score: 4/5
- In
frontend/app/src/entities/branches/ui/branches-provider.test.tsx, the test named “keeps its children mounted while the branches are refetched in the background” doesn’t actually trigger or observe a refetch path, so it can pass even if background refetch behavior regresses and children unmount unexpectedly in production. Update the test to simulate a real refetch transition (including relevant loading state changes) and assert children stay mounted during that cycle.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/app/src/entities/branches/ui/branches-provider.test.tsx">
<violation number="1" location="frontend/app/src/entities/branches/ui/branches-provider.test.tsx:120">
P2: The "keeps its children mounted while the branches are refetched in the background" test does not exercise any refetch behavior: BranchesProvider destructures only `isPending` and `error` from useGetBranches and never reads `isFetching`, so the newly added `isFetching: true` flag is inert. With data present and `isPending: false` the provider returns children regardless, so this assertion is indistinguishable from the first default-branch test and gives no regression confidence for the refetch scenario it claims to cover.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Refactor deriving the branch from the URL, removing the duplicated atom and fixing the hardcoded-'main' render guard; the new provider tests pin the corrected behavior. No external contract, data, or operational tradeoff remains for human judgment.
Re-trigger cubic
What changed and why
BranchesProviderheld the selected branch in two places — thebranchquery string parameter andcurrentBranchAtom— and kept them in step with an effect. Two representations of one fact meant the provider had to block rendering while they disagreed, and the branch selector had to write both on every switch.The query string parameter is now the single source of truth.
currentBranchis derived during render:currentBranchAtomis deleted.setCurrentBranchstays on the context but writes the query string parameter, so the "default branch means no parameter" convention lives in one place instead of being re-derived by each caller.Fixes a latent bug
The removed render guard compared the resolved branch against a hardcoded
"main":The branch itself is resolved by
is_default, butINFRAHUB_INITIAL_DEFAULT_BRANCHlets a deployment name its default branch anything. On such a deployment the comparison never succeeded and the app never rendered past "Loading branches...". Deriving the branch removes the comparison entirely.Five other hardcoded
"main"checks remain out of scope here (shared/config/config.ts,branches-to-select-options.ts,branches-table.tsx,diff/ui/checks/data-conflict.tsx, andDEFAULT_BRANCH_NAMEitself) —config.tsstill falls back to/graphql/main, so those deployments need a follow-up.Notable implementation details
navigate("/"). It no longer writes state, so it cannot re-trigger itself.isFieldDisabledwas reaching into the global Jotai store for one boolean. It now takesisDefaultBranch, threaded fromgetFormFieldsFromSchemathroughgetFormFieldFromAttributeto the 8 form call sites, and is a pure function again. The relationship builder passespermissions: undefined, so the branch bit is unreachable there and is not threaded.store.set(currentBranchAtom, ...)in favour of a plain argument, and the component-test wrapper no longer needsuseStateto fake the branch.Testing performed
Behaviour verified by hand against the existing e2e coverage of the unknown-branch redirect (
tests/e2e/branches/branch-selector.spec.tsandtests/e2e/branches/test_branch_selector.py), which asserts the toast, theOtherbutton and the cleared URL — all preserved. Branch creation was checked for a regression risk:useCreateBranchMutationawaitsrefetchQueries(["branches"])in its ownonSuccess, which React Query runs before the per-call callback, so the new branch is in the cache before the query string parameter flips and no spurious not-found toast fires.🤖 Generated with Claude Code