Add variable UI#1251
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds variable metadata and UI plus renames template flag: Variable gains nullable description and creation (UserAndTimestamp); services record creation and adjust permissions; applyTemplate renamed to renderTemplate across server/client; tests updated; frontend adds variable CRUD UI, RTK Query endpoints, and preview-with-variables support. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapp/src/dogma/features/repo/settings/RepositorySettingsView.tsx (1)
38-55: Verify anonymous access for repo variables.
allowAnonymous: trueon the variables tab may leak repo-level variable values. If variables can contain secrets/config, disable anonymous access.🔒 Suggested adjustment
- { name: 'variables', path: 'variables', accessRole: 'WRITE', allowAnonymous: true }, + { name: 'variables', path: 'variables', accessRole: 'WRITE', allowAnonymous: false },
🤖 Fix all issues with AI agents
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/Variable.java`:
- Around line 120-122: In Variable.equals(...) replace the direct call
creation.equals(variable.creation) with a null-safe comparison using
Objects.equals(creation, variable.creation) so the nullable field annotated
`@Nullable` is compared safely; update the equals method in class Variable to use
Objects.equals for the creation field (keeping existing comparisons for non-null
fields like value as-is).
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/VariableServiceV1.java`:
- Around line 231-241: The helper setNameAndCreation currently overwrites the
variable.creation on both create and update; split responsibilities by adding a
new helper setName(Variable variable, String projectName, `@Nullable` String
repoName) that only builds and sets the name, and keep
setNameAndCreation(Variable..., Author) for creation flows; update all update*
handlers to call setName instead of setNameAndCreation, and ensure create*
handlers continue to call setNameAndCreation (or alternatively, guard inside
setNameAndCreation to only set creation if variable.getCreation() == null or
rename creation to lastModified if overwriting was intended).
In `@webapp/src/dogma/features/project/settings/ProjectSettingsView.tsx`:
- Around line 37-45: The variables tab currently allows anonymous access which
may leak sensitive values; locate the 'variables' tab configuration in
ProjectSettingsView.tsx (related to the TabName union and the tab options around
the allowAnonymous flag) and change its access control so it no longer sets
allowAnonymous: true unless you intentionally want public access — instead set
allowAnonymous: false or replace with an authenticated-role check (e.g., require
specific roles or isAuthenticated) for the variables tab to ensure only
authorized users can view variable values.
In `@webapp/src/dogma/features/project/settings/variables/VariableForm.tsx`:
- Around line 176-188: The JSON Textarea in the VariableForm component is
missing a defaultValue so edit mode doesn't pre-populate the field; update the
Textarea (id="value-json") to pass defaultValue using the existing
defaultValue.value (or JSON.stringify(defaultValue.value) if the stored value is
an object) and keep the existing react-hook-form register validation
(register('value', ...)) so the textarea shows the current variable value when
editing.
- Around line 214-217: The Create button in the VariableForm component doesn't
reflect the submission state; update it to mirror the Update button by passing
the same loading prop used there (e.g., isLoading={isSubmitting} or
loading={isSubmitting}) and keep loadingText="Creating", and also disable the
button while submitting (disabled={isSubmitting}) to prevent double-submission —
locate the Button inside the isNew branch in VariableForm and apply the same
loading/disabled logic used by the Update button.
- Around line 198-206: In VariableForm update the description Textarea so its id
and name reflect the description field (currently id="value" and name="value"):
change them to id="description" and name="description" to match
register('description') and defaultValue.description; this fixes accessibility
and avoids confusion between the value and description inputs.
- Around line 142-152: The loop callback parameter shadows the component state
variable named variableType causing the defaultChecked comparison to be wrong;
in the VARIABLE_TYPES.map callback rename the parameter (e.g., option or
typeOption) to avoid shadowing, and update the Radio's defaultChecked to compare
the loop item's type to the component state (variableType) or the intended
selected value (e.g., defaultChecked={variableType === option.type} or checked
based on the form value) while keeping the register('type') usage and
defaultValue reference correct.
In `@webapp/src/dogma/features/project/settings/variables/VariableList.tsx`:
- Around line 69-76: Accessor callbacks for the VariableDto columns assume
creation is always present and will throw when creation is undefined; update the
columnHelper.accessor calls (the ones that read creation.user and
creation.timestamp) to safely handle missing creation by using optional chaining
(e.g., row.creation?.user and row.creation?.timestamp) and return sensible
fallbacks in the cell renderers (e.g., empty string or "—" for Text and a
null-safe DateWithTooltip prop) so the UI does not error when creation is
undefined.
In `@webapp/src/dogma/features/project/settings/variables/VariableView.tsx`:
- Around line 160-175: The code reads variable.creation.user and
variable.creation.timestamp directly but VariableDto marks creation as optional;
update the VariableView rendering to guard against undefined creation: check
variable.creation before accessing .user and before passing .timestamp to
DateWithTooltip (e.g., render a fallback like "-" or "Unknown" for user and only
render DateWithTooltip when creation?.timestamp exists). Apply these checks
where variable.creation is referenced in VariableView (around the Modified By
and Modified At rows) so null/undefined creation won't cause a runtime error.
- Around line 83-94: The PrismJS version used in VariableView.tsx (where
Prism.highlight is called inside the render/format helper that returns a <Code
... dangerouslySetInnerHTML={{ __html: Prism.highlight(...) }} />) is
vulnerable; update the PrismJS dependency in package.json to "prismjs":
">=1.30.0" (or bump to latest), run your package manager to refresh lockfile
(npm install / yarn install / pnpm install), and rebuild; ensure the updated
lockfile/lock entries are included in the PR so CI picks up the patched Prism
(no code changes to VariableView.tsx required other than dependency bump).
In
`@webapp/src/pages/app/projects/`[projectName]/repos/[repoName]/settings/variables/[id]/index.tsx:
- Around line 25-31: RepoVariableViewPage fires useGetVariableQuery with invalid
params because router.query can be undefined on first render; update the
component to wait for router.isReady before calling the query by passing a
boolean skip (or `skip: !router.isReady`) or conditional call to
useGetVariableQuery so it only runs when router.isReady is true and
projectName/repoName/id are non-empty; reference the component name
(RepoVariableViewPage), the router.isReady flag, and the hook
useGetVariableQuery to locate where to add the guard.
In
`@webapp/src/pages/app/projects/`[projectName]/repos/[repoName]/settings/variables/new/index.tsx:
- Around line 43-55: The onSubmit handler contains an unreachable error check
after calling addNewVariable(...).unwrap(); remove the redundant block that
inspects (response as { error: ... }).error and the throw, since unwrap()
already throws on error; instead treat response as the successful payload and
proceed to dispatch newNotification, call onSuccess(), and Router.push; update
references in this function (onSubmit, addNewVariable, unwrap, dispatch,
newNotification, Router.push) so the code flow assumes unwrap either returns the
success response or throws.
In
`@webapp/src/pages/app/projects/`[projectName]/settings/variables/[id]/edit/index.tsx:
- Around line 39-45: The onSubmit handler currently mutates the incoming
VariableDto and contains an unreachable post-unwrap error check; instead, stop
mutating the parameter and build a new payload object (e.g., const payload = {
...variable, name: `projects/${projectName}/variables/${variable.id}` }) and
pass that to updateVariable({ projectName, id, variable: payload }). Remove the
if-check that inspects response.error after calling .unwrap() since unwrap
throws on errors and will be handled by the catch block.
In
`@webapp/src/pages/app/projects/`[projectName]/settings/variables/[id]/index.tsx:
- Around line 25-30: The page calls useGetVariableQuery with router.query values
before Next's router is ready causing invalid requests; update VariableViewPage
to wait for router.isReady and only call useGetVariableQuery when ready by
deriving projectName and id after router.isReady and passing the RTK Query hook
a skip option (or conditional) so useGetVariableQuery({ projectName, id }, {
skip: !router.isReady }) is used until router.isReady is true.
🧹 Nitpick comments (8)
webapp/src/pages/app/projects/[projectName]/settings/variables/index.tsx (1)
28-29: Consider skipping the query whenprojectNameis empty.When the router isn't ready,
projectNamewill be an empty string, causing an unnecessary API call. Consider using RTK Query'sskipoption to prevent this.♻️ Suggested improvement
- const projectName = router.query.projectName ? (router.query.projectName as string) : ''; - const { data: variablesData } = useGetVariablesQuery({ projectName }); + const projectName = router.query.projectName as string; + const { data: variablesData } = useGetVariablesQuery({ projectName }, { skip: !projectName });webapp/src/pages/app/projects/[projectName]/settings/variables/[id]/edit/index.tsx (1)
31-33: Consider guarding against undefined router params.Direct casting without checking could cause issues when the router isn't ready (e.g., during initial render). The query will also fire with undefined values.
♻️ Suggested improvement
const router = useRouter(); - const projectName = router.query.projectName as string; - const id = router.query.id as string; + const projectName = (router.query.projectName as string) || ''; + const id = (router.query.id as string) || ''; - const { data, isLoading: isVariableLoading, error } = useGetVariableQuery({ projectName, id }); + const { data, isLoading: isVariableLoading, error } = useGetVariableQuery( + { projectName, id }, + { skip: !projectName || !id } + );webapp/src/pages/app/projects/[projectName]/repos/[repoName]/settings/variables/index.tsx (1)
28-30: Consider skipping the query when params are empty.Same as the project-level page—when the router isn't ready, queries will fire with empty strings.
♻️ Suggested improvement
- const projectName = router.query.projectName ? (router.query.projectName as string) : ''; - const repoName = router.query.repoName ? (router.query.repoName as string) : ''; - const { data: variablesData } = useGetVariablesQuery({ projectName, repoName }); + const projectName = router.query.projectName as string; + const repoName = router.query.repoName as string; + const { data: variablesData } = useGetVariablesQuery( + { projectName, repoName }, + { skip: !projectName || !repoName } + );webapp/src/dogma/features/project/settings/variables/VariableList.tsx (1)
26-41: Remove unused generic type parameter.The generic
Data extends objectis unused—the component only works withVariableDto. The eslint-disable comment confirms this is dead code.♻️ Suggested simplification
-// eslint-disable-next-line `@typescript-eslint/no-unused-vars` -export type VariableListProps<Data extends object> = { +export type VariableListProps = { projectName: string; repoName?: string; variables: VariableDto[]; deleteVariable: (projectName: string, id: string, repoName?: string) => Promise<void>; isLoading: boolean; }; -const VariableList = <Data extends object>({ +const VariableList = ({ projectName, repoName, variables, deleteVariable, isLoading, -}: VariableListProps<Data>) => { +}: VariableListProps) => {webapp/src/pages/app/projects/[projectName]/settings/variables/new/index.tsx (2)
45-47: Redundant error check afterunwrap().RTK Query's
unwrap()already throws the error if the mutation fails, so the response will only contain the actual data on success. This error check will never be true.🔧 Suggested fix
try { const response = await addNewVariable({ projectName, newVariable }).unwrap(); - if ((response as { error: FetchBaseQueryError | SerializedError }).error) { - throw (response as { error: FetchBaseQueryError | SerializedError }).error; - } dispatch(newNotification('New variable is created', `Successfully created`, 'success'));
17-17: Inconsistent router usage.The code imports both
Router(default) anduseRouter, then usesrouterfrom the hook on Line 36 butRouter.pushon Line 50. Consider usingrouter.pushconsistently since the router instance is already available.🔧 Suggested fix
-import Router, { useRouter } from 'next/router'; +import { useRouter } from 'next/router';Then on Line 50:
- Router.push(`/app/projects/${projectName}/settings/variables`); + router.push(`/app/projects/${projectName}/settings/variables`);Also applies to: 50-50
webapp/src/pages/app/projects/[projectName]/repos/[repoName]/settings/variables/[id]/edit/index.tsx (2)
43-45: Redundant error check afterunwrap().Same as noted in the project-level new variable page - RTK Query's
unwrap()throws on error, so this check is unreachable.🔧 Suggested fix
try { const response = await updateVariable({ projectName, repoName, id, variable }).unwrap(); - if ((response as { error: FetchBaseQueryError | SerializedError }).error) { - throw (response as { error: FetchBaseQueryError | SerializedError }).error; - } dispatch(newNotification(`Variable '${variable.id}' is updated`, `Successfully updated`, 'success'));
17-17: Inconsistent router usage.Same pattern as the project-level page - imports both
RouteranduseRouter, but could userouter.pushconsistently.Also applies to: 48-48
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/TemplateParamsConverter.java`:
- Around line 43-48: The code in TemplateParamsConverter currently reads only
ctx.queryParam("renderTemplate") which breaks clients still sending
applyTemplate; update the parsing in the TemplateParamsConverter so it accepts
both names: first check ctx.queryParam("renderTemplate") and if null fallback to
ctx.queryParam("applyTemplate"), parse the chosen value with
Boolean.parseBoolean (default false when null), keep variableFile and
templateRevision logic the same (templateRevStr -> new Revision(...)), and call
TemplateParams.of(renderTemplate, variableFile, templateRevision); ensure the
preference order uses renderTemplate when both are present.
In `@webapp/src/dogma/common/components/editor/FileEditor.tsx`:
- Around line 126-139: modifiedContent can be left undefined when the preview
query is skipped or the editor hasn't mounted; ensure you always assign a safe
fallback (e.g., empty string) so the preview remains stable. Update the
assignment logic for modifiedContent in FileEditor.tsx to use nullish coalescing
or explicit fallbacks: use dataWithVariables?.rawContent ?? '' when readOnly and
no error, use `ErrorMessageParser.parse(error)` guarded and default to `''` if
parsing fails, and use `editorRef?.current?.getValue() ?? ''` when not readOnly.
Apply the same pattern to the other occurrence referenced (around lines 250-265)
so both code paths always produce a defined string.
- Line 147: Remove the debug console.log that prints file contents in
FileEditor.tsx (the line logging fileContent, originalContent, revision).
Replace it by either removing the statement entirely or, if runtime tracing is
needed, log only non-sensitive metadata (e.g., file identifier or revision) and
never raw fileContent/originalContent; update any tests or callers that relied
on that console output accordingly.
- Around line 141-145: The current render block mutates state directly by
comparing cachedRevision to revision and calling setFileContent and
setCachedRevision; move this synchronization out of render into a useEffect hook
that depends on revision (and optionally displayContent) so the effect runs when
revision changes, and inside the effect call setFileContent(displayContent) and
setCachedRevision(String(revision)); reference the existing variables
cachedRevision, setCachedRevision, setFileContent, revision and displayContent
and implement the logic inside useEffect in the FileEditor component instead of
in the render body.
In `@webapp/src/dogma/features/file/NewFile.tsx`:
- Line 38: The FILE_PATH_PATTERN currently only anchors the end of the string,
allowing prefixes like "../secret" to match; update the regex used in
NewFile.tsx (symbol FILE_PATH_PATTERN) to enforce full-string validation by
adding a start anchor so the pattern matches the entire input (i.e., change
/[-_.0-9a-zA-Z]*[-_0-9a-zA-Z]+$/ to include a leading ^ to become fully
anchored).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/java/src/main/java/com/linecorp/centraldogma/client/AbstractFileRequest.java (1)
50-77:⚠️ Potential issue | 🟠 MajorEnsure this breaking API change is documented in release notes and migration guide.
The methods
applyTemplate(boolean)andapplyTemplate(String)have been replaced withrenderTemplate(...). All existing usages in the codebase have been updated, but external consumers will need to update their code when upgrading. Document this breaking change in the release notes and provide a migration guide to help users transition to the new method names.
🧹 Nitpick comments (4)
webapp/src/dogma/features/api/apiSlice.ts (1)
563-602: Consider request-specific DTOs for variable create/update.
VariableDtoincludes fields that are often server-managed (e.g.,creation, possiblyid). If those are read-only, a dedicatedCreateVariableRequestDto/UpdateVariableRequestDto(omitting read-only fields) reduces validation risk and payload size.server/src/test/java/com/linecorp/centraldogma/server/internal/api/variable/VariableServiceV1Test.java (2)
39-50: Align MOCK_CREATION with the authenticated userUsing a hardcoded
"tester"can drift from the USERNAME used by the auth setup if you later assert creation metadata. Consider deriving it from USERNAME for consistency.♻️ Suggested tweak
-private static final UserAndTimestamp MOCK_CREATION = new UserAndTimestamp("tester"); +private static final UserAndTimestamp MOCK_CREATION = new UserAndTimestamp(USERNAME);
96-98: Add at least one assertion for creation metadataAll comparisons currently ignore
creation, so regressions in server-side creation (user/timestamp) won’t be caught. Consider asserting it in one create/read/update path.Also applies to: 115-117, 127-129, 138-141, 149-152, 182-185
server/src/test/java/com/linecorp/centraldogma/server/VariableTemplateCrudTest.java (1)
56-56: Omit client-supplied creation metadata in test requestsThe service unconditionally overwrites creation with server-managed values (VariableServiceV1.java line 239), making client-supplied creation in tests unnecessary. Remove the
UserAndTimestampimport and passnullfor the creation parameter in Variable constructors to clarify test intent and avoid duplicating server-side logic.♻️ Proposed adjustment
-import com.linecorp.centraldogma.server.metadata.UserAndTimestamp;- final UserAndTimestamp creation = new UserAndTimestamp("tester"); - final Variable variable = new Variable(id, type, null, value, creation, null); + final Variable variable = new Variable(id, type, null, value, null, null);
Contributor
Author
|
Screenshots: |
jrhee17
approved these changes
Feb 2, 2026
|
|
||
| <FormControl isRequired isInvalid={errors.value != null}> | ||
| <FormLabel> | ||
| <LabelledIcon icon={variableType === 'STRING' ? CiText : TbJson} text="Value" /> |
Contributor
There was a problem hiding this comment.
Note) For the record, I'm still unsure why a separate String type is necessary when the UI could map variable name to each json key.
Contributor
Author
There was a problem hiding this comment.
There is a slight difference in perspective on this. We may need to look at real use cases later, but personally, I don't think JSON encoding in Web UI without IDE assistance is convenient.
To provide a better user experience, I thought offering a STRING, so users can simply enter values in an input box without worrrying about encoding, would be helpful. If we only provide JSON type, users would have to add quotes even for simple value, which could make the UI inconvenient.
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
2 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:
As a final step in template file support, add a UI for manipulating variables so users can manage variables and preview rendered files in the Central Dogma webapp.
Modifications:
Preview with Variablestab in the file viewer to preview the rendered file with applied variables.applyTemplatetorenderTemplate, which seems to convey the intention more clearly for both the method and parameter name.MEMBERandWRITEfromProjectRole.OWNERandRepositoryRole.ADMIN..variables.xxxfile, so granting the same permission seems to make sense.localhost:3000to127.0.0.1:3000to avoid cross-origin issues in development environments.Result:
You can now manage variables using the variable UI in Central Dogma web app.