Open
Conversation
Motivation: Rolling back a repository is currently manual and error-prone, and the history view has no direct revert action. If a wrong configuration is deployed, users may want a single-click rollback to recover quickly. Modifications: - Add a revert API that rebuilds the target snapshot via upserts/removals using raw content comparison. - Expose a root-only Revert action with a modal and API wiring in the web UI. - Add server, unit, and e2e coverage for the revert flow. Result: - Users can revert a repository to a chosen revision directly from the history page. - Closes line#298
📝 WalkthroughWalkthroughImplements a repository revert feature enabling rollback to specific commits. Introduces a RevertRequest DTO, a new REST endpoint that compares target and head revisions to compute change sets, a frontend modal for user confirmation, and comprehensive test coverage spanning backend integration, frontend unit, and end-to-end tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/UI
participant Modal as RevertCommitModal
participant API as Frontend API
participant Backend as ContentServiceV1
participant Repo as Repository
User->>Modal: Open modal & set targetRevision
Modal->>Modal: Initialize form with<br/>prefilled commit message
User->>Modal: Submit revert request
Modal->>API: Call useRevertRepositoryMutation
API->>Backend: POST /revert<br/>(targetRevision, commitMessage)
Backend->>Repo: Fetch target entries
Backend->>Repo: Fetch head entries
Backend->>Backend: Compare content &<br/>build change set
alt Changes exist
Backend->>Repo: Push new commit
Backend-->>API: Return PushResultDto
API-->>Modal: Success
Modal->>User: Show success notification
else No changes
Backend-->>API: RedundantChangeException
API-->>Modal: Conflict
Modal->>User: Show no-changes notification
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java`:
- Around line 325-337: The method toUpsertChange currently throws a
java.lang.Error for unexpected entry types; replace that with throwing an
IllegalStateException (preserving the existing message) so application-level
unexpected states are signaled correctly; update the throw in toUpsertChange
(and keep existing behavior for switch cases and the assert on
entry.rawContent()) to throw new IllegalStateException("unexpected entry type: "
+ entry.type()) instead of new Error(...).
- Around line 317-323: Replace the runtime-unsafe assert in isSameContent with
explicit null checks: use Objects.requireNonNull(first.rawContent(),
"first.rawContent() must not be null") and
Objects.requireNonNull(second.rawContent(), "second.rawContent() must not be
null") (import java.util.Objects) before comparing via equals; update the
isSameContent(Entry<?> first, Entry<?> second) method to perform these checks so
nulls are handled deterministically in production.
- Around line 259-315: The revert method currently calls the private push(...)
helper directly and skips meta-repo validation; before invoking push in the
thenCompose block, call the existing checkMetaRepoPush(repository, changes)
validation (same one used by the public /contents endpoint) using the collected
changes to ensure protected repos like "dogma"/"meta" cannot be reverted to
disallowed states, and only then proceed to call push(...). Ensure you reference
the local variables used there (changes, repository, author, headRevision,
request.commitMessage(), commitTimeMillis) so the validation runs
synchronously/throws before push is executed.
In `@webapp/e2e/revert.spec.ts`:
- Around line 3-35: The test hard-codes API_BASE which uses a different
origin/port and breaks cookie/CSRF context; remove or stop using the top-level
API_BASE constant and instead derive the API base inside apiPost (or before
calling it) from the current page origin (e.g. use new URL(await
page.url()).origin) and concatenate that origin with the path when building the
fetch URL; update the apiPost function (and any callers) so the evaluated fetch
uses the derived origin + path rather than the hard-coded API_BASE to ensure
cookies and CSRF tokens match the request origin.
🧹 Nitpick comments (2)
.gitignore (1)
143-145: Unrelated change: Consider separating into its own commit.This
.gitignoreaddition for/AGENTS.mdappears unrelated to the revert feature. While harmless, bundling unrelated changes makes it harder to track the history of changes.webapp/src/dogma/features/history/RevertCommitModal.tsx (1)
73-90: Simplify error handling - remove dead code.
- Lines 75-77: After
.unwrap(), errors are thrown automatically, so theresponse.errorcheck is never reached.- Lines 78-81: The backend throws
RedundantChangeException(HTTP 409) when no changes exist, which is caught in thecatchblock. The API returns aPushResultDtoon success, notnull.♻️ Proposed simplification
const onSubmit = async (formData: FormData) => { const data = { targetRevision, commitMessage: { summary: formData.summary, detail: formData.detail, }, }; try { - const response = await revertRepository({ projectName, repoName, data }).unwrap(); - if ((response as { error: FetchBaseQueryError | SerializedError }).error) { - throw (response as { error: FetchBaseQueryError | SerializedError }).error; - } - if (response === null) { - dispatch( - newNotification('No changes to revert', `Repository is already at r${targetRevision}`, 'info'), - ); - } else { - dispatch(newNotification('Repository reverted', `Reverted to r${targetRevision}`, 'success')); - } + await revertRepository({ projectName, repoName, data }).unwrap(); + dispatch(newNotification('Repository reverted', `Reverted to r${targetRevision}`, 'success')); reset(); onClose(); } catch (error) { dispatch(newNotification(`Failed to revert`, ErrorMessageParser.parse(error), 'error')); } };
server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java
Show resolved
Hide resolved
Comment on lines
+317
to
+323
| private static boolean isSameContent(Entry<?> first, Entry<?> second) { | ||
| if (first.type() != second.type()) { | ||
| return false; | ||
| } | ||
| assert first.rawContent() != null && second.rawContent() != null; | ||
| return first.rawContent().equals(second.rawContent()); | ||
| } |
There was a problem hiding this comment.
Avoid assert for null checks in production code.
Assertions can be disabled at runtime with the -da JVM flag. Use explicit null checks or requireNonNull for production-critical validations.
🔧 Proposed fix
private static boolean isSameContent(Entry<?> first, Entry<?> second) {
if (first.type() != second.type()) {
return false;
}
- assert first.rawContent() != null && second.rawContent() != null;
+ if (first.rawContent() == null || second.rawContent() == null) {
+ return false;
+ }
return first.rawContent().equals(second.rawContent());
}🤖 Prompt for AI Agents
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java`
around lines 317 - 323, Replace the runtime-unsafe assert in isSameContent with
explicit null checks: use Objects.requireNonNull(first.rawContent(),
"first.rawContent() must not be null") and
Objects.requireNonNull(second.rawContent(), "second.rawContent() must not be
null") (import java.util.Objects) before comparing via equals; update the
isSameContent(Entry<?> first, Entry<?> second) method to perform these checks so
nulls are handled deterministically in production.
server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java
Show resolved
Hide resolved
jrhee17
approved these changes
Feb 5, 2026
| public CompletableFuture<PushResultDto> revert( | ||
| Repository repository, Author author, RevertRequest request) { | ||
| requireNonNull(request, "request"); | ||
| final Revision headRevision = repository.normalizeNow(Revision.HEAD); |
Contributor
There was a problem hiding this comment.
Question) Would it make sense to apply the revert within the lock? The following scenario seems counter-intuitive to me
- Revert request A and push request B is received simultaneously
- request A fetches the head as version (n)
- request B wins and a push is applied first, now the repo is version (n + 1)
- the revert is computed to target (n - k), only diffs between (n - k)..(n) is computed
- the revert is pushed, applying the revert of (n - k)..(n) on top of (n + 1)
- given that overwriting methods are used (
upsert,removal), no conflict and the push goes through.
I don't think it makes much sense that a revert operation throws a conflict exception either
Contributor
Author
There was a problem hiding this comment.
given that overwriting methods are used (upsert, removal), no conflict and the push goes through.
The head revision (n) is specified for the revert commit.
Therefore, if the head revision is advanced by request B, the operation will fail with
ChangeConflictException. Do you think it is inappropriate for a revert operation to throw ChangeConflictException?
Contributor
There was a problem hiding this comment.
The head revision (n) is specified for the revert commit.
I see, I think I missed this point. In that case, I think the current changes make sense to me.
minwoox
approved these changes
Feb 27, 2026
| return false; | ||
| } | ||
| assert first.rawContent() != null && second.rawContent() != null; | ||
| return first.rawContent().equals(second.rawContent()); |
Contributor
There was a problem hiding this comment.
Should we do this in the blocking task executor instead of the repository worker? And is there any chance that this is executed by the event loop when CachingRepository is involved?
Contributor
Author
There was a problem hiding this comment.
Do you think this comparison is too expensive to run on the event loop? I didn't understand why we need to offload the computation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java (3)
304-314:⚠️ Potential issue | 🟠 MajorMissing
checkMetaRepoPushvalidation before push.The revert endpoint bypasses the
checkMetaRepoPushvalidation that protectsdogmaandmetarepositories from unauthorized changes. This allows reverting to states that may include disallowed files.🛡️ Proposed fix
}).thenCompose(changes -> { if (changes.isEmpty()) { throw new RedundantChangeException(headRevision, "No changes to revert. Target revision(" + targetRevision.major() + ") is the same as HEAD(" + headRevision.major() + ")."); } + checkMetaRepoPush(repository.name(), changes); final long commitTimeMillis = System.currentTimeMillis(); return push(commitTimeMillis, author, repository, headRevision, request.commitMessage(), changes) .thenApply(rrev -> DtoConverter.newPushResultDto(rrev, commitTimeMillis)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java` around lines 304 - 314, The revert flow in ContentServiceV1 skips the repository protection check and should call the existing checkMetaRepoPush validation before performing push; in the lambda where you currently compute commitTimeMillis and call push(...), invoke checkMetaRepoPush(repository, changes) (or the appropriate signature used elsewhere) and handle its failure before calling push, so protected repositories like "dogma" and "meta" are validated prior to push and the subsequent DtoConverter.newPushResultDto call.
325-337:⚠️ Potential issue | 🟠 MajorReplace
assertwith explicit null check and useIllegalStateExceptioninstead ofError.The
asserton line 326 can be disabled at runtime. TheErroron line 335 is typically reserved for JVM-level problems; application-level unexpected states should useIllegalStateException.🔧 Proposed fix
private static Change<?> toUpsertChange(Entry<?> entry) { - assert entry.rawContent() != null; + if (entry.rawContent() == null) { + throw new IllegalStateException("Entry has no content: " + entry.path()); + } switch (entry.type()) { case JSON: return Change.ofJsonUpsert(entry.path(), entry.rawContent()); case YAML: return Change.ofYamlUpsert(entry.path(), entry.rawContent()); case TEXT: return Change.ofTextUpsert(entry.path(), entry.rawContent()); default: - throw new Error("unexpected entry type: " + entry.type()); + throw new IllegalStateException("Unexpected entry type: " + entry.type()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java` around lines 325 - 337, In toUpsertChange(Entry<?> entry) replace the runtime-optional assert with an explicit null check for entry.rawContent() and throw an IllegalStateException if it is null; also change the fallback throw from new Error(...) to throw new IllegalStateException("unexpected entry type: " + entry.type()) so application-level unexpected states use IllegalStateException; keep existing switch behavior for JSON/YAML/TEXT and ensure the null-check is done before the switch.
317-323:⚠️ Potential issue | 🟠 MajorReplace
assertwith explicit null check.Assertions can be disabled at runtime with the
-daJVM flag, making this validation unreliable in production.🔧 Proposed fix
private static boolean isSameContent(Entry<?> first, Entry<?> second) { if (first.type() != second.type()) { return false; } - assert first.rawContent() != null && second.rawContent() != null; + if (first.rawContent() == null || second.rawContent() == null) { + return false; + } return first.rawContent().equals(second.rawContent()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java` around lines 317 - 323, The method isSameContent uses an assertion to check rawContent() non-null which can be disabled; replace the assert with an explicit null-safe comparison: ensure you still compare types via isSameContent(Entry<?> first, Entry<?> second) -> check first.type() != second.type() first, then handle nulls from first.rawContent() or second.rawContent() (e.g. if either is null return false or use Objects.equals(first.rawContent(), second.rawContent()) to safely compare), referencing the Entry<?> rawContent() and type() methods so no assertion is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java`:
- Around line 269-273: The revert endpoint currently constructs a Revision from
request.targetRevision() without validating it, causing new Revision(...) to
throw for the default 0 value; update ContentServiceV1 to validate
RevertRequest.targetRevision before calling new Revision: in the method handling
reverts (taking Repository, Author, RevertRequest), check that
request.targetRevision() is present and > 0 (or reject missing/zero with a
400/IllegalArgumentException) and return an appropriate API error; only call
repository.normalizeNow(new
Revision(Integer.toString(request.targetRevision()))) when the targetRevision is
a valid positive integer.
---
Duplicate comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java`:
- Around line 304-314: The revert flow in ContentServiceV1 skips the repository
protection check and should call the existing checkMetaRepoPush validation
before performing push; in the lambda where you currently compute
commitTimeMillis and call push(...), invoke checkMetaRepoPush(repository,
changes) (or the appropriate signature used elsewhere) and handle its failure
before calling push, so protected repositories like "dogma" and "meta" are
validated prior to push and the subsequent DtoConverter.newPushResultDto call.
- Around line 325-337: In toUpsertChange(Entry<?> entry) replace the
runtime-optional assert with an explicit null check for entry.rawContent() and
throw an IllegalStateException if it is null; also change the fallback throw
from new Error(...) to throw new IllegalStateException("unexpected entry type: "
+ entry.type()) so application-level unexpected states use
IllegalStateException; keep existing switch behavior for JSON/YAML/TEXT and
ensure the null-check is done before the switch.
- Around line 317-323: The method isSameContent uses an assertion to check
rawContent() non-null which can be disabled; replace the assert with an explicit
null-safe comparison: ensure you still compare types via isSameContent(Entry<?>
first, Entry<?> second) -> check first.type() != second.type() first, then
handle nulls from first.rawContent() or second.rawContent() (e.g. if either is
null return false or use Objects.equals(first.rawContent(), second.rawContent())
to safely compare), referencing the Entry<?> rawContent() and type() methods so
no assertion is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61286d55-0a39-40f6-a8e9-0366dfa109cf
📒 Files selected for processing (1)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java
Comment on lines
+269
to
+273
| Repository repository, Author author, RevertRequest request) { | ||
| requireNonNull(request, "request"); | ||
| final Revision headRevision = repository.normalizeNow(Revision.HEAD); | ||
| final Revision targetRevision = | ||
| repository.normalizeNow(new Revision(Integer.toString(request.targetRevision()))); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how Revision handles "0" or invalid values
ast-grep --pattern $'new Revision($_)'
rg -n "class Revision" --type java -A 30 | head -60Repository: line/centraldogma
Length of output: 50373
🏁 Script executed:
#!/bin/bash
# Find and examine the Revision class definition
fd -t f "Revision.java" --path "*/common/src/main/java*" | head -1 | xargs cat -n | head -100Repository: line/centraldogma
Length of output: 294
🏁 Script executed:
#!/bin/bash
# Find Revision.java file
fd "Revision.java" "common/src/main/java" -t f
# Then examine the class definition
if [ -f "common/src/main/java/com/linecorp/centraldogma/common/Revision.java" ]; then
cat -n "common/src/main/java/com/linecorp/centraldogma/common/Revision.java" | head -150
fiRepository: line/centraldogma
Length of output: 6241
🏁 Script executed:
#!/bin/bash
# Find RevertRequest definition to check default value behavior
fd "RevertRequest.java" -t fRepository: line/centraldogma
Length of output: 142
🏁 Script executed:
#!/bin/bash
# Examine RevertRequest definition
cat -n "common/src/main/java/com/linecorp/centraldogma/internal/api/v1/RevertRequest.java"Repository: line/centraldogma
Length of output: 2875
Add input validation for targetRevision in revert endpoint.
When targetRevision is omitted from the JSON payload, it defaults to 0. Passing this to new Revision(...) throws an IllegalArgumentException since Revision rejects zero. Validate the value at the API layer instead:
Proposed fix
public CompletableFuture<PushResultDto> revert(
Repository repository, Author author, RevertRequest request) {
requireNonNull(request, "request");
+ if (request.targetRevision() == 0) {
+ throw new IllegalArgumentException("targetRevision must be specified");
+ }
final Revision headRevision = repository.normalizeNow(Revision.HEAD);
final Revision targetRevision =
repository.normalizeNow(new Revision(Integer.toString(request.targetRevision())));📝 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
| Repository repository, Author author, RevertRequest request) { | |
| requireNonNull(request, "request"); | |
| final Revision headRevision = repository.normalizeNow(Revision.HEAD); | |
| final Revision targetRevision = | |
| repository.normalizeNow(new Revision(Integer.toString(request.targetRevision()))); | |
| Repository repository, Author author, RevertRequest request) { | |
| requireNonNull(request, "request"); | |
| if (request.targetRevision() == 0) { | |
| throw new IllegalArgumentException("targetRevision must be specified"); | |
| } | |
| final Revision headRevision = repository.normalizeNow(Revision.HEAD); | |
| final Revision targetRevision = | |
| repository.normalizeNow(new Revision(Integer.toString(request.targetRevision()))); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java`
around lines 269 - 273, The revert endpoint currently constructs a Revision from
request.targetRevision() without validating it, causing new Revision(...) to
throw for the default 0 value; update ContentServiceV1 to validate
RevertRequest.targetRevision before calling new Revision: in the method handling
reverts (taking Repository, Author, RevertRequest), check that
request.targetRevision() is present and > 0 (or reject missing/zero with a
400/IllegalArgumentException) and return an appropriate API error; only call
repository.normalizeNow(new
Revision(Integer.toString(request.targetRevision()))) when the targetRevision is
a valid positive integer.
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
3 participants
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.
Motivation:
Rolling back a repository is currently manual and error-prone, and the history view has no direct revert action. If a wrong configuration is deployed, users may want a single-click rollback to recover quickly.
Modifications:
Result: