feat: support multi-branching within Studio #16970
Conversation
WalkthroughAdds branch-management APIs and models to backend, implements source-control operations (create, checkout with validation, current-branch, discard local changes), propagates uncommitted-change errors, and exposes corresponding frontend types, hooks, components, and tests to list/create/switch branches and handle uncommitted changes. Duplicate controller methods noted in the diff. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend
participant Backend
participant Git
User->>Frontend: Request branch checkout
Frontend->>Backend: POST /repos/{org}/{repo}/checkout (branch)
Backend->>Backend: CheckoutBranchWithValidation -> RepositoryStatus check
alt Uncommitted changes
Backend-->>Frontend: 409 + UncommittedChangesError
Frontend->>Frontend: Show UncommittedChangesDialog
User->>Frontend: Discard & Switch
Frontend->>Backend: POST /repos/{org}/{repo}/discard-changes
Backend->>Git: Hard reset & remove untracked
Backend-->>Frontend: RepoStatus
Frontend->>Backend: POST /repos/{org}/{repo}/checkout (branch)
Backend->>Git: Fetch & checkout branch
Backend-->>Frontend: RepoStatus
else No uncommitted changes
Backend->>Git: Fetch & checkout branch
Backend-->>Frontend: RepoStatus
end
sequenceDiagram
actor User
participant Frontend
participant Backend
participant Git
User->>Frontend: Create branch (name)
Frontend->>Frontend: Validate name (BranchNameValidator)
alt Invalid name
Frontend->>Frontend: Show validation error
else Valid name
Frontend->>Backend: POST /repos/{org}/{repo}/branches (branch)
Backend->>Git: Create branch ref
Backend-->>Frontend: Branch object
Frontend->>Backend: POST /repos/{org}/{repo}/checkout (branch)
Backend->>Git: Checkout branch
Backend-->>Frontend: RepoStatus
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
area/version-control
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16970 +/- ##
==========================================
- Coverage 96.19% 96.09% -0.10%
==========================================
Files 2447 2407 -40
Lines 30790 30648 -142
Branches 3544 3532 -12
==========================================
- Hits 29618 29451 -167
- Misses 864 887 +23
- Partials 308 310 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (13)
src/Designer/frontend/packages/shared/src/mocks/queriesMock.ts (1)
243-243: Consider returning a proper RepoStatus mock.The checkoutBranch mock currently returns an empty object, but the actual return type should be RepoStatus with fields like currentBranch, contentStatus, repositoryStatus, etc. Consider providing a mock with appropriate default values to improve test reliability and prevent potential undefined property access in tests.
Apply this diff to provide a more complete mock:
- checkoutBranch: jest.fn().mockImplementation(() => Promise.resolve({})), + checkoutBranch: jest.fn().mockImplementation(() => Promise.resolve<RepoStatus>({ + currentBranch: 'master', + contentStatus: [], + repositoryStatus: 'Clean', + hasMergeConflict: false, + })),src/Designer/frontend/packages/shared/src/hooks/queries/useCurrentBranchQuery.ts (1)
7-16: Consider adding optional query options parameter.The hook currently doesn't accept any query options (e.g.,
enabled,staleTime,refetchInterval). Consider adding an optionaloptionsparameter to allow consumers to customise the query behaviour when needed, following the pattern used in other query hooks in the codebase.Apply this diff to add optional parameters:
export const useCurrentBranchQuery = ( org: string, app: string, + options?: UseQueryOptions<CurrentBranchInfo>, ): UseQueryResult<CurrentBranchInfo> => { const { getCurrentBranch } = useServicesContext(); return useQuery<CurrentBranchInfo>({ queryKey: [QueryKey.CurrentBranch, org, app], queryFn: () => getCurrentBranch(org, app), + ...options, }); };src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/BranchDropdown/BranchDropdown.tsx (1)
52-52: Consider using 'main' as the default branch name.The code defaults to
'master'if current branch information is unavailable. Modern Git repositories typically use'main'as the default branch name. Consider updating this default or making it configurable.- const currentBranch = currentBranchInfo?.branchName || 'master'; + const currentBranch = currentBranchInfo?.branchName || 'main';src/Designer/frontend/packages/shared/src/components/UncommittedChangesDialog/UncommittedChangesDialog.tsx (1)
38-42: Consider query invalidation instead of hard page reload.Using
location.reload()causes a hard refresh that loses all application state. Consider whether invalidating relevant queries (CurrentBranch,RepoStatus,Branches) would be sufficient to update the UI without disrupting the user experience.const checkoutMutation = useCheckoutBranchMutation(org, app, { onSuccess: () => { - location.reload(); + queryClient.invalidateQueries({ queryKey: [QueryKey.CurrentBranch, org, app] }); + queryClient.invalidateQueries({ queryKey: [QueryKey.RepoStatus, org, app] }); + onClose(); }, });Note: This requires access to
queryClientviauseQueryClient()hook.src/Designer/frontend/packages/shared/src/hooks/mutations/useDiscardChangesMutation.ts (1)
18-21: Consider invalidating CurrentBranch query for consistency.Whilst
RepoStatusis invalidated after discarding changes, theCurrentBranchquery might also benefit from invalidation to ensure UI consistency. Other mutation hooks (e.g.,useCheckoutBranchMutation) invalidate both queries.onSuccess: (data, variables, context) => { queryClient.invalidateQueries({ queryKey: [QueryKey.RepoStatus, org, app] }); + queryClient.invalidateQueries({ queryKey: [QueryKey.CurrentBranch, org, app] }); options?.onSuccess?.(data, variables, context); },src/Designer/frontend/packages/shared/src/api/mutations.ts (1)
4-6: Branch mutations look correct; consider adding generics for stronger typingThe new
createBranch,checkoutBranch, anddiscardChangesmutations line up with the backend routes andBranch/RepoStatusshapes, and the{ branchName }payload matches the request models, so the behaviour looks sound. For extra type-safety you could mirror nearby functions and specify the return type onpost, e.g.post<Branch>(...)/post<RepoStatus>(...), so TypeScript enforces alignment between the declaredPromise<…>type and the actual response.Also applies to: 81-82, 212-215
src/Designer/frontend/packages/shared/src/hooks/mutations/useCheckoutWithUncommittedChangesHandling.ts (1)
7-33: Hook logic is solid; you can drop theas UseMutationOptionscastThe wrapper correctly routes
409responses with data toonUncommittedChangesand all other failures toonOtherError, while leaving the baseuseCheckoutBranchMutationto handle cache invalidation. To tighten types, you can avoid theas UseMutationOptions<…>cast by matching the callback signatures that TanStack Query expects:- return useCheckoutBranchMutation(org, app, { - onSuccess: () => { - onSuccess?.(); - }, - onError: (error: AxiosError<UncommittedChangesError>) => { - if (HttpResponseUtils.isConflict(error) && error.response?.data) { - onUncommittedChanges(error.response.data); - } else { - onOtherError?.(error); - } - }, - retry: false, - } as UseMutationOptions<RepoStatus, AxiosError, string>); + return useCheckoutBranchMutation(org, app, { + onSuccess: (_data, _variables, _context) => { + onSuccess?.(); + }, + onError: (error: AxiosError<UncommittedChangesError>, _variables, _context) => { + if (HttpResponseUtils.isConflict(error) && error.response?.data) { + onUncommittedChanges(error.response.data); + } else { + onOtherError?.(error); + } + }, + retry: false, + });This keeps the same runtime behaviour but lets TypeScript validate the option shape without assertions.
src/Designer/backend/src/Designer/Controllers/RepositoryController.cs (1)
418-464: Confirm cloning behaviour for new branch endpoints
RepoStatusexplicitly calls_sourceControl.CloneIfNotExistsandFetchRemoteChangesbefore reading status, butGetCurrentBranch,CheckoutBranch, andDiscardLocalChangesdelegate straight to the source control implementation. If these endpoints can be hit before any local clone exists,LibGit2Sharp.Repositoryusage in the underlying service may throw.It may be worth either:
- Ensuring the source control methods used here internally perform a
CloneIfNotExists/fetch, or- Mirroring the
RepoStatuspattern in these actions before calling into_sourceControl.Please double-check the expected call sequence and whether a missing local clone is possible for these routes.
src/Designer/frontend/packages/shared/src/hooks/useCreateBranchFlow.test.ts (1)
451-504: Reduce flakiness in loading-state tests by avoiding realsetTimeoutThe loading tests currently rely on
setTimeoutin the mockedcreateBranch, which introduces real time into the tests and can make them slower or flaky under load.You can make these tests more deterministic by using a manually controlled Promise or Jest fake timers instead of
setTimeout, for example by returning a Promise that you resolve from the test body once you’ve assertedisCreatingOrCheckingOut === true.src/Designer/frontend/packages/shared/src/utils/BranchNameValidator.ts (1)
1-32: Validation logic looks solid; consider narrowingerrorKeytypeThe validation steps and use of
GIT_BRANCH_VALIDATIONlook correct and easy to follow.For additional type safety you could narrow
ValidationResult.errorKeyfromstringto a union of the known keys (e.g.'create_branch.error_empty' | 'create_branch.error_invalid_chars' | 'create_branch.error_invalid_pattern' | 'create_branch.error_reserved_ending' | '') so that typos in callers are caught at compile time.src/Designer/backend/src/Designer/Services/Implementation/SourceControlLoggingDecorator.cs (1)
396-436: Delegation looks correct; tweakLogErrorparameters for clearer logsThe new wrappers correctly delegate to the underlying
ISourceControland follow the existing try/catch logging pattern.In
CheckoutBranchWithValidation, theLogErrorcall passesrepositoryas both the repository and the destination path:LogError(ex, "CheckoutBranchWithValidation", org, repository, repository, branchName);This will log the repository name twice, which is slightly confusing. You could pass an empty destination instead to better reflect the method signature:
- LogError(ex, "CheckoutBranchWithValidation", org, repository, repository, branchName); + LogError(ex, "CheckoutBranchWithValidation", org, repository, string.Empty, branchName);src/Designer/backend/tests/Designer.Tests/Controllers/RepositoryController/CreateBranchTests.cs (1)
92-105: Assert that no source-control call is made for an empty request
CreateBranch_EmptyRequest_ReturnsNoContentverifies the 204 response but does not currently assert thatISourceControl.CreateBranchwas not invoked, even though the comment says it “should not create branch”.You could strengthen the test by adding a verification:
- using HttpResponseMessage response = await HttpClient.PostAsync(uri, content); - - // Assert - // Empty body with null BranchName doesn't cause error, but should not create branch - Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + using HttpResponseMessage response = await HttpClient.PostAsync(uri, content); + + // Assert + // Empty body with null BranchName doesn't cause error, but should not create branch + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + _sourceControlMock.Verify( + x => x.CreateBranch(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>()), + Times.Never);src/Designer/backend/src/Designer/Models/BranchModels.cs (1)
1-1: Consider enabling nullable reference types for this new file.The
#nullable disabledirective is present on a new file. Based on Altinn Studio conventions, new code should embrace nullable reference types for better null-safety. This would allow you to explicitly mark which properties can be null (e.g.,RemoteNameinCurrentBranchInfo) and which cannot (e.g.,BranchNamein request models), improving type safety and API clarity.Apply this diff to enable nullable reference types and add appropriate annotations:
-#nullable disable +#nullable enable namespace Altinn.Studio.Designer.Models { /// <summary> /// Request model for creating a new branch /// </summary> public class CreateBranchRequest { /// <summary> /// Gets or sets the name of the branch to create /// </summary> - public string BranchName { get; set; } + public required string BranchName { get; set; } } /// <summary> /// Request model for checking out a branch /// </summary> public class CheckoutBranchRequest { /// <summary> /// Gets or sets the name of the branch to checkout /// </summary> - public string BranchName { get; set; } + public required string BranchName { get; set; } } /// <summary> /// Information about the current branch /// </summary> public class CurrentBranchInfo { /// <summary> /// Gets or sets the name of the current branch /// </summary> - public string BranchName { get; set; } + public required string BranchName { get; set; } /// <summary> /// Gets or sets the SHA of the current commit /// </summary> - public string CommitSha { get; set; } + public string? CommitSha { get; set; } /// <summary> /// Gets or sets whether the branch is tracking a remote branch /// </summary> public bool IsTracking { get; set; } /// <summary> /// Gets or sets the name of the tracked remote branch /// </summary> - public string RemoteName { get; set; } + public string? RemoteName { get; set; } } // Similar changes for UncommittedChangesError and UncommittedFile... }
| /// <summary> | ||
| /// Creates a new branch in the repository | ||
| /// </summary> | ||
| /// <param name="org">Unique identifier of the organisation responsible for the app.</param> | ||
| /// <param name="repository">The name of repository</param> | ||
| /// <param name="request">The branch creation request</param> | ||
| /// <returns>The created branch</returns> | ||
| [HttpPost] | ||
| [Route("repo/{org}/{repository:regex(^(?!datamodels$)[[a-z]][[a-z0-9-]]{{1,28}}[[a-z0-9]]$)}/branches")] | ||
| public async Task<ActionResult<Branch>> CreateBranch(string org, string repository, [FromBody] CreateBranchRequest request) | ||
| { | ||
| var branch = await _sourceControl.CreateBranch(org, repository, request.BranchName); | ||
| return Ok(branch); | ||
| } |
There was a problem hiding this comment.
Add basic validation for CreateBranch request to avoid null/empty branch names
CreateBranch currently assumes request and request.BranchName are non-null and non-empty; sending an empty body or whitespace branch name will either cause a null-reference or push validation down into _sourceControl.CreateBranch, likely surfacing as a 500. CheckoutBranch already guards this case.
Consider aligning CreateBranch with CheckoutBranch by validating upfront:
- public async Task<ActionResult<Branch>> CreateBranch(string org, string repository, [FromBody] CreateBranchRequest request)
- {
- var branch = await _sourceControl.CreateBranch(org, repository, request.BranchName);
- return Ok(branch);
- }
+ public async Task<ActionResult<Branch>> CreateBranch(string org, string repository, [FromBody] CreateBranchRequest request)
+ {
+ if (string.IsNullOrWhiteSpace(request?.BranchName))
+ {
+ return BadRequest("Branch name is required");
+ }
+
+ var branch = await _sourceControl.CreateBranch(org, repository, request.BranchName);
+ return Ok(branch);
+ }This keeps the API surface consistent and prevents avoidable 500s on bad input.
🤖 Prompt for AI Agents
In src/Designer/backend/src/Designer/Controllers/RepositoryController.cs around
lines 403 to 416, CreateBranch does not validate the request or
request.BranchName and can throw a null-reference or surface a 500 for
empty/whitespace input; update the method to mirror CheckoutBranch by checking
if request is null or string.IsNullOrWhiteSpace(request.BranchName) and return
BadRequest("Invalid branch name") (or the same message used by CheckoutBranch)
before calling _sourceControl.CreateBranch, ensuring consistent API behavior and
avoiding server errors on bad input.
src/Designer/backend/src/Designer/Filters/Git/GitExceptionFilterAttribute.cs
Show resolved
Hide resolved
src/Designer/backend/src/Designer/Services/Implementation/SourceControlSI.cs
Show resolved
Hide resolved
| const checkoutMutation = useCheckoutWithUncommittedChangesHandling(org, app, { | ||
| onUncommittedChanges: (error) => { | ||
| setUncommittedChangesError(error); | ||
| }, | ||
| }); | ||
|
|
||
| const handleBranchSelect = (branchName: string) => { | ||
| if (!branchName || branchName === currentBranchInfo?.branchName) return; | ||
|
|
||
| setTargetBranchForSwitch(branchName); | ||
| checkoutMutation.mutate(branchName); | ||
| }; |
There was a problem hiding this comment.
Add onSuccess handler to reload the page after successful checkout.
The checkout mutation is missing an onSuccess callback, which means the page won't reload after a successful branch switch. This is inconsistent with the UncommittedChangesDialog component (line 40 in UncommittedChangesDialog.tsx), which reloads the page after checkout. Without the reload, users may not see the updated application state for the new branch.
const checkoutMutation = useCheckoutWithUncommittedChangesHandling(org, app, {
onUncommittedChanges: (error) => {
setUncommittedChangesError(error);
},
+ onSuccess: () => {
+ location.reload();
+ },
});🤖 Prompt for AI Agents
In
src/Designer/frontend/packages/shared/src/components/GiteaHeader/VersionControlButtons/components/BranchDropdown/BranchDropdown.tsx
around lines 26 to 37, the checkout mutation setup lacks an onSuccess handler so
the page doesn't reload after a successful branch checkout; update the options
passed to useCheckoutWithUncommittedChangesHandling to include an onSuccess
callback that triggers a full reload (e.g. window.location.reload()) after
successful checkout and clear any UI state (like targetBranchForSwitch) as
needed to match UncommittedChangesDialog behavior.
| export const GIT_BRANCH_VALIDATION = { | ||
| INVALID_CHARS: /[\s~^:?*[\]\\@{]/, | ||
| INVALID_PATTERNS: /(\.\.|\/\/|^\/|\/$)/, | ||
| RESERVED_ENDING: /\.lock$/, | ||
| } as const; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify branch name validation rules are complete.
The validation patterns cover common Git branch name restrictions, but the completeness should be verified against Git's official branch naming rules. Consider checking:
- Ending with
.(period) - Control characters (ASCII < 32, DEL 127)
- The single character
@ - Additional sequences beyond those listed
🌐 Web query:
What are the complete Git branch naming rules and restrictions?
💡 Result:
Short answer — use git-check-ref-format to validate, and follow these rules (from Git docs):
- May include slash (/) for hierarchy, but no path component may:
- begin with a dot (.).
- end with the sequence ".lock".
- Must not contain consecutive slash components (no "//").
- Must not start with "refs/".
- Must not start with a dash (-) or a dot (.).
- Must not end with a slash (/) or with the sequences ".git" or ".lock".
- Must not contain the sequence ".." (double dot).
- Must not contain the sequence "@{".
- Must not be the single character "@".
- Must not contain any ASCII control characters (0x00–0x1f) or DEL (0x7f).
- Must not contain any of these characters: space, tilde (~), caret (^), colon (:), question mark (?), asterisk (*), open bracket ([), or backslash ().
- Branch names are case‑sensitive in Git (but some filesystems are case‑insensitive — avoid names differing only by case).
- Unicode is allowed but may cause tool/terminal compatibility issues.
Command to check a candidate branch name:
git check-ref-format --branch
Sources: Git documentation (git-check-ref-format / refname rules). [1][2][3]
References
- Git documentation: git-check-ref-format / refname rules. [1]
- Git project documentation (git-check-ref-format.txt). [2]
- Practical summary / examples (git user docs / tutorials). [3]
Add missing Git branch name validation rules.
The validation patterns are incomplete against Git's official branch naming rules. Gaps found:
- Missing ASCII control character validation (0x00–0x1f, 0x7f)
- Missing start-of-name restrictions: dash (−) and dot (.)
- Missing end-of-name restriction: ".git" suffix
- Overly restrictive character validation: @ and { are flagged individually, but Git forbids only the sequence "@{" and @ only as a complete branch name
Update the regex patterns in src/Designer/frontend/packages/shared/src/constants/gitBranchValidation.ts (lines 1–5) to align with Git's complete refname rules. Consider separate patterns for start restrictions, end restrictions, control characters, and the "@{" sequence.
🤖 Prompt for AI Agents
In src/Designer/frontend/packages/shared/src/constants/gitBranchValidation.ts
lines 1–5, the current regexes miss several Git refname rules: add a
CONTROL_CHARS pattern to catch ASCII control chars (0x00–0x1F and 0x7F), add a
START_INVALID pattern that forbids branch names beginning with dash or dot, add
an END_INVALID pattern that forbids suffixes like ".git" (and keep ".lock"), and
replace the "@"/"{" checks with a specific AT_CURLY pattern that rejects the
sequence "@{". Also tighten/adjust the general INVALID_CHARS pattern to remove
individual @ and { (so @ alone is allowed except when name is exactly "@") and
include remaining illegal characters/spaces; update the exported
GIT_BRANCH_VALIDATION object to contain these separate patterns (CONTROL_CHARS,
START_INVALID, INVALID_CHARS, AT_CURLY, END_INVALID) so validation uses them
together.
src/Designer/frontend/packages/shared/src/hooks/useCreateBranchFlow.test.ts
Show resolved
Hide resolved
…hFlow.test.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Designer/frontend/packages/shared/src/hooks/useCreateBranchFlow.test.ts (2)
193-194: Consider using.mockResolvedValue()for clearer mock syntax.Throughout the test file, some mocks use
.mockImplementation(() => Promise.resolve(...))whilst others use the more concise.mockResolvedValue(...)(e.g., lines 399-400). For consistency and readability, prefer.mockResolvedValue()and.mockRejectedValue().Apply this pattern:
- const createBranch = jest.fn().mockImplementation(() => Promise.resolve(mockBranch)); - const checkoutBranch = jest.fn().mockImplementation(() => Promise.resolve(mockRepoStatus)); + const createBranch = jest.fn().mockResolvedValue(mockBranch); + const checkoutBranch = jest.fn().mockResolvedValue(mockRepoStatus);Also applies to: 216-217
271-283: Consider extracting AxiosError creation into a helper function.AxiosError mock objects are duplicated across multiple tests with similar structure. Extracting this into a helper function would improve maintainability and reduce duplication.
Example helper at the top of the test file:
const createMockAxiosError = (status: number, data: any = {}): AxiosError => ({ response: { status, data, statusText: status === 409 ? 'Conflict' : 'Internal Server Error', headers: {}, config: {} as any, }, isAxiosError: true, toJSON: () => ({}), name: 'AxiosError', message: `Request failed with status code ${status}`, });Then use it:
const error409 = createMockAxiosError(409); const error500 = createMockAxiosError(500);Also applies to: 310-322, 359-371, 419-431, 526-538
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Designer/frontend/packages/shared/src/hooks/useCreateBranchFlow.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 16095
File: src/App/app-template-dotnet/src/App/models/model.cs:10-29
Timestamp: 2025-08-15T05:53:32.529Z
Learning: When moving app templates in Altinn Studio, mirkoSekulic prefers to keep the PR scope strictly limited to the move operation itself, with all content changes (including code quality improvements, naming convention fixes, and attribute cleanup) deferred to separate PRs for better maintainability and atomic changes.
Learnt from: framitdavid
Repo: Altinn/altinn-studio PR: 16372
File: src/Designer/frontend/libs/studio-components/src/components/index.ts:34-34
Timestamp: 2025-09-23T08:53:19.508Z
Learning: In the Altinn Studio codebase, when moving components from studio-components-legacy to studio-components, the team prefers to handle the component migration and remaining import updates in separate PRs to maintain focused, atomic changes.
Learnt from: TomasEng
Repo: Altinn/altinn-studio PR: 0
File: :0-0
Timestamp: 2025-09-10T12:02:06.348Z
Learning: In the Altinn Studio repository, the team convention is to remove irrelevant checkboxes from the PR template description rather than leaving them unmarked. This makes it clear that the author has intentionally considered each item and removed non-applicable ones, helping to avoid merging PRs with incomplete checklists.
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 16191
File: .dockerignore:3-3
Timestamp: 2025-08-26T12:35:44.069Z
Learning: In the Altinn Studio repository, mirkoSekulic is implementing a phased directory restructuring approach where backend and testdata are moved first, followed by separate moves for readme, frontend, and development directories into the Designer directory. This ensures atomic changes and prevents path reference issues during the transition.
Learnt from: mgunnerud
Repo: Altinn/altinn-studio PR: 16187
File: frontend/resourceadm/components/ResourcePageInputs/ResourceSwitchInput.tsx:3-3
Timestamp: 2025-08-25T13:55:27.042Z
Learning: In the Altinn Studio codebase, when migrating from studio/components-legacy to studio/components, PRs should focus only on components that have available replacements in the new package. Components without new implementations should remain in the legacy package until replacements are available, supporting an incremental migration approach.
Learnt from: ErlingHauan
Repo: Altinn/altinn-studio PR: 15977
File: frontend/libs/studio-components-legacy/src/components/StudioCard/index.tsx:2-7
Timestamp: 2025-08-06T12:37:01.381Z
Learning: When reviewing PRs in the Altinn Studio repository where a developer copies a component from `studio-components-legacy` to `studio-components`, suggest adding a `deprecated` JSDoc comment to the component in `studio-components-legacy` if one hasn't been added. The deprecation comment should recommend using the component from `studio/components` instead.
📚 Learning: 2025-08-08T13:24:30.117Z
Learnt from: nkylstad
Repo: Altinn/altinn-studio PR: 0
File: :0-0
Timestamp: 2025-08-08T13:24:30.117Z
Learning: In Altinn/altinn-studio PR reviews, when unit tests need React Query/TanStack Query data, prefer seeding with QueryClient.setQueryData over rendering/mocking use…Query hooks and waiting for isSuccess. Flag patterns like renderHookWithMockStore(() => use…Query(...)) and waitFor(() => expect(result.current.isSuccess).toBe(true)) and recommend the simpler setQueryData approach. Cite the internal Confluence guideline on component tests and the best-practice example in useAddItemToLayoutMutation.test.ts (lines updated 08.08.2025).
Applied to files:
src/Designer/frontend/packages/shared/src/hooks/useCreateBranchFlow.test.ts
🧬 Code graph analysis (1)
src/Designer/frontend/packages/shared/src/hooks/useCreateBranchFlow.test.ts (4)
src/Designer/frontend/libs/studio-components/src/types/KeyValuePairs.ts (1)
KeyValuePairs(1-3)src/Designer/frontend/packages/shared/src/types/api/BranchTypes.ts (3)
Branch(1-7)RepoStatus(37-47)UncommittedChangesError(29-35)src/Designer/frontend/packages/shared/src/api/mutations.ts (2)
createBranch(213-213)checkoutBranch(214-214)src/Designer/frontend/packages/shared/src/hooks/useCreateBranchFlow.ts (1)
useCreateBranchFlow(28-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Run integration tests against actual gitea and dbs
- GitHub Check: CodeQL
- GitHub Check: Testing
- GitHub Check: Analyze
- GitHub Check: Typechecking and linting
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
🔇 Additional comments (2)
src/Designer/frontend/packages/shared/src/hooks/useCreateBranchFlow.test.ts (2)
398-416: Test correctly implements onSuccess verification.The previous issue has been resolved. This test now properly sets the branch name, triggers the create flow, and asserts that the
onSuccesscallback is invoked after successful checkout.
1-588: Comprehensive test coverage with solid structure.This test suite provides excellent coverage of the
useCreateBranchFlowhook, testing initialisation, validation, mutation flows, error handling, loading states, and dialog interactions. The tests follow React Testing Library best practices and properly useact()andwaitFor()for async operations.
| /// <summary> | ||
| /// Gets or sets the list of uncommitted files | ||
| /// </summary> | ||
| public System.Collections.Generic.List<UncommittedFile> UncommittedFiles { get; set; } |
There was a problem hiding this comment.
Nit: If you add using System.Collections.Generic; at the top of the file, this can be shortened:
| public System.Collections.Generic.List<UncommittedFile> UncommittedFiles { get; set; } | |
| public List<UncommittedFile> UncommittedFiles { get; set; } |
| @@ -6,5 +6,7 @@ public class GitErrorCodes | |||
| public const string NonFastForwardError = "GT_01"; | |||
| public const string RepositoryNotFound = "GT_02"; | |||
| public const string GiteaSessionExpired = "GT_03"; | |||
| public const string UncommittedChanges = "GT_04"; | |||
| public const string BranchNotFound = "GT_05"; | |||
There was a problem hiding this comment.
Nit: Looks like BranchNotFound is not used. I suggest implementing or removing the definition.
| @@ -294,6 +302,9 @@ public RepoStatus RepositoryStatus(string org, string repository) | |||
| repoStatus.AheadBy = branch.TrackingDetails.AheadBy; | |||
| repoStatus.BehindBy = branch.TrackingDetails.BehindBy; | |||
| } | |||
|
|
|||
| // Add current branch name | |||
There was a problem hiding this comment.
Nit: This comment may be superflous.
There was a problem hiding this comment.
Vurdér om vi skal bruke norsk "Gren" i stedet for "Branch".
There was a problem hiding this comment.
Det skal ikke være mellomrom mellom "Branch" og "navn".
For eksempel kan "Branch navn kan ikke være tomt" omskrives til "Branch-navnet kan ikke være tomt".
| }, | ||
| ); | ||
|
|
||
| expect(result.current.createButtonText).toBe('[mockedText(create_branch.create)]'); |
There was a problem hiding this comment.
Nit: I think this is the pattern we normally use:
| expect(result.current.createButtonText).toBe('[mockedText(create_branch.create)]'); | |
| expect(result.current.createButtonText).toBe(textMock('create_branch.create')); |
Goes throughout this file.
| if (context.Exception is Exceptions.UncommittedChangesException uncommittedChangesException) | ||
| { | ||
| context.Result = new ObjectResult(uncommittedChangesException.ErrorDetails) { StatusCode = (int)HttpStatusCode.Conflict }; | ||
| context.ExceptionHandled = true; |
There was a problem hiding this comment.
Have you considered if the line context.ExceptionHandled = true may prevent locating the exception in debugging situations?
2 participants
Description
Verification
Summary by CodeRabbit