feat: optimize tags input #593
Conversation
🦋 Changeset detectedLatest commit: 7d0913e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. WalkthroughReplaces string[] tags with TagItem[] (value + isInputting), refactors tag creation/confirmation/removal and template rendering, updates tests to assert form values, switches Jest to zone test environment and suppresses specific CSS errors, and adjusts release workflows/scripts and package repository metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Input as Input Element
participant Component as TagsInputComponent
participant Form as FormControl
User->>Input: type characters
Input->>Component: onInput(value)
Component->>Component: create/ensure TagItem(isInputting=true)
Component->>Form: emitModel(TagItem[])
User->>Input: press Enter / blur
Input->>Component: onKeyDown / onBlur
Component->>Component: confirmInput() -> set isInputting=false
Component->>Form: emitModel(TagItem[])
User->>Component: click remove icon
Component->>Component: onRemove(index) -> remove TagItem
Component->>Form: emitModel(TagItem[])
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 2
🤖 Fix all issues with AI agents
In @src/input/tags-input/tags-input.component.html:
- Line 12: The @for block uses an invalid track expression "track index"; change
the track clause to use the implicit $index variable (i.e., "track $index") and,
if you need a local alias, keep the let binding "let index = $index" so the loop
becomes @for (item of model; track $index; let index = $index) {; update the
@for declaration accordingly.
In @src/input/tags-input/tags-input.component.ts:
- Around line 272-288: The current valueIn method uses index-based matching to
preserve isInputting which breaks when the incoming array is reordered/modified;
update valueIn in tags-input.component (function valueIn, referencing model,
isInputting, clearInput, sortByReadonly) to preserve editing state by matching
existing model entries by value (or a unique id if available) instead of by
index, and add unit tests covering external reordering, removal, and addition of
tags while editing to validate the new behavior.
🧹 Nitpick comments (1)
src/input/tags-input/tags-input.component.ts (1)
211-243: Complex input state management - consider extraction.The logic correctly manages the inputting tag state and clears stale validation errors when the user modifies input. However, the method handles multiple concerns (state updates, error clearing, width calculation).
Consider extracting the inputting item state update logic (lines 215-223) into a helper method like
updateInputtingItem(value: string)to improve readability.♻️ Optional refactor to extract inputting item logic
onInput() { const value = this.inputRef.nativeElement.value; - const lastItem = this.model[this.model.length - 1]; - - if (lastItem?.isInputting) { - this.model = value - ? [...this.model.slice(0, -1), { value, isInputting: true }] - : this.model.slice(0, -1); - } else if (value) { - this.model = [...this.model, { value, isInputting: true }]; - } + this.updateInputtingItem(value); this.emitModel(this.model); if ( this.controlContainer?.control?.errors?.[INPUT_ERROR_KEY] && Object.keys(this.controlContainer.control.errors).length === 1 ) { this.controlContainer.control.setErrors(null); } requestAnimationFrame(() => { if (value.length) { this.renderer.setStyle( this.inputRef.nativeElement, 'width', this.inputValueMirror.nativeElement.scrollWidth + 'px', ); } else { this.renderer.removeStyle(this.inputRef.nativeElement, 'width'); } }); } + + private updateInputtingItem(value: string) { + const lastItem = this.model[this.model.length - 1]; + + if (lastItem?.isInputting) { + this.model = value + ? [...this.model.slice(0, -1), { value, isInputting: true }] + : this.model.slice(0, -1); + } else if (value) { + this.model = [...this.model, { value, isInputting: true }]; + } + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/input/tags-input/tags-input.component.htmlsrc/input/tags-input/tags-input.component.ts
⏰ 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). (5)
- GitHub Check: Release Beta
- GitHub Check: Unit Test
- GitHub Check: Redirect rules - aui
- GitHub Check: Header rules - aui
- GitHub Check: Pages changed - aui
🔇 Additional comments (8)
src/input/tags-input/tags-input.component.html (1)
13-25: LGTM!The conditional rendering of confirmed tags (filtering out
isInputtingitems) aligns well with the refactored data model. The references toitem.valueare consistent throughout the template.src/input/tags-input/tags-input.component.ts (7)
37-54: LGTM!The
TagIteminterface cleanly separates confirmed tags from in-progress input, and the standalone component configuration is correct.
64-64: LGTM!The dual type parameters elegantly separate the external API (
string[]) from the internal model (TagItem[]), and theconfirmedTagsgetter provides a clean way to access only the confirmed tags for validation.Also applies to: 141-143
202-209: LGTM!The method correctly handles the
TagItemmodel, properly checkingtarget.valueagainstreadonlyTags.
245-270: LGTM!The keyboard interaction (Backspace/Enter) and blur handling correctly delegate to the
confirmInputmethod, maintaining consistent validation behavior.
290-311: LGTM!The
modelOuttransformation is straightforward, andsortByReadonlyefficiently uses aSetfor O(1) readonly tag lookup while maintaining the desired order (readonly tags first).
313-325: LGTM!The helper methods cleanly handle the two input confirmation outcomes (removal vs. confirmation) with proper cleanup and model emission.
327-351: LGTM!The confirmation flow correctly validates against
confirmedTags(excluding inputting items) and properly handles both synchronous and asynchronous validation with thetake(1)pattern.
| > | ||
| {{ tag }} | ||
| </aui-tag> | ||
| @for (item of model; track index; let index = $index) { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Angular @for track syntax correct usage documentation
💡 Result:
Short answer — use the @for block with a required track expression inside the parentheses:
Syntax examples
-
Track by item property (recommended for dynamic lists):
@for (item of items; track item.id) { - {{ item.name }} }
-
Track by index (for static lists):
@for (item of items; track $index) { - {{ item }} }
Notes
- The track expression is required for @for and should uniquely identify each item to minimize DOM operations.
- Inside the block you can use contextual variables like $index, $first, $last, $even, $odd and alias them with let.
- You may add an @empty block that renders when the collection is empty.
References
- @for API docs. [1]
- Control flow guide (explanation of track and examples). [2]
Sources:
[1] https://angular.dev/api/core/%40for
[2] https://angular.dev/guide/templates/control-flow
Fix the track expression syntax for Angular @for block.
The track index syntax is incorrect. The track expression must reference $index directly, not a plain index identifier. The correct syntax is track $index.
If you want to alias $index as index for use within the block, use:
@for (item of model; track $index; let index = $index) {
The track expression is evaluated before let bindings, so only implicit variables like $index are available in the track clause.
🤖 Prompt for AI Agents
In @src/input/tags-input/tags-input.component.html at line 12, The @for block
uses an invalid track expression "track index"; change the track clause to use
the implicit $index variable (i.e., "track $index") and, if you need a local
alias, keep the let binding "let index = $index" so the loop becomes @for (item
of model; track $index; let index = $index) {; update the @for declaration
accordingly.
fengtianze
force-pushed
the
feat/optimize-tags-input
branch
from
fad72ce to
8be091a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/input/tags-input/tags-input.component.html (1)
12-12: Consider tracking byitem.valueinstead ofindexfor better performance.When items are added or removed from the middle of the list, tracking by index causes Angular to re-render more elements than necessary. If tag values are unique, tracking by value avoids unnecessary DOM operations.
♻️ Suggested improvement
- @for (item of model; let index = $index; track index) { + @for (item of model; let index = $index; track item.value) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/input/tags-input/tags-input.component.htmlsrc/input/tags-input/tags-input.component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/input/tags-input/tags-input.component.ts
⏰ 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). (1)
- GitHub Check: Unit Test
🔇 Additional comments (1)
src/input/tags-input/tags-input.component.html (1)
13-25: LGTM!The conditional rendering logic correctly filters out items in the "inputting" state, and the tag bindings properly reference
item.valuefrom the newTagItemstructure.
fengtianze
force-pushed
the
feat/optimize-tags-input
branch
from
8be091a to
0b95847
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/input/tags-input/tags-input.component.ts:
- Around line 272-288: In valueIn, the current index-based preservation of
isInputting can misalign when this.model differs in length/order; update valueIn
to map incoming v to TagItem by finding the corresponding entry in this.model by
value (e.g., locate this.model.find(m => m.value === value)) and use its
isInputting if found, otherwise false; after building items keep the existing
check that calls clearInput() when no item.isInputting, and return the result
through sortByReadonly—refer to valueIn, this.model, clearInput, sortByReadonly,
and TagItem when making the change.
- Around line 343-350: The code watches this.inputControl.pending and then on
statusChanges calls syncControlStatus() and confirmInputtingItem() if valid, but
it doesn’t handle the case when the async validator resolves to invalid; update
the subscription in the pending branch to check this.inputControl.valid and call
confirmInputtingItem() as now, and add an else branch that removes the
transient/inputting item from the model (the same model mutated by
confirmInputtingItem()) — you can call the existing removal logic or a helper
(e.g., removeInputtingItem or the code path that cancels input) after
syncControlStatus() when this.inputControl.valid is false to ensure the
inputting item is not left indefinitely.
🧹 Nitpick comments (2)
src/input/tags-input/tags-input.component.html (1)
12-26: Consider using a stable identity fortrackinstead of index.Using
track indexcan cause incorrect DOM reuse when items are removed or reordered (e.g., when readonly tags are sorted to the front). Consider tracking byitem.valueor a unique identifier for more predictable behavior.♻️ Suggested change
- @for (item of model; let index = $index; track index) { + @for (item of model; let index = $index; track item.value) {src/input/tags-input/tags-input.component.ts (1)
211-243: Complex input handling logic is correct but could benefit from extraction.The
onInputmethod handles multiple concerns: updating the inputting item, emitting the model, clearing errors, and resizing the input. Consider extracting the error-clearing logic (lines 225-230) into a separate method for clarity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
jest.setup.tssrc/input/tags-input/tags-input-form.component.spec.tssrc/input/tags-input/tags-input.component.htmlsrc/input/tags-input/tags-input.component.spec.tssrc/input/tags-input/tags-input.component.ts
⏰ 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). (1)
- GitHub Check: Unit Test
🔇 Additional comments (5)
jest.setup.ts (1)
1-15: LGTM! Jest setup modernization with appropriate error suppression.The migration to
setupZoneTestEnv()aligns with current jest-preset-angular patterns. The CSS parsing error suppression is well-scoped and preserves other console errors.src/input/tags-input/tags-input.component.spec.ts (1)
37-47: LGTM! Test now correctly simulates real user input flow.The addition of
dispatchEvent(new Event('input'))before the Enter keydown accurately reflects actual user behavior where the input value must first be captured before confirmation.src/input/tags-input/tags-input-form.component.spec.ts (1)
139-155: Verify:allowEmpty=truetest expects empty array for empty input.Both
allowEmpty=falseandallowEmpty=truetests result in the same empty array. This appears intentional since an empty input with no actual text shouldn't create a tag, but please confirm this matches the expected behavior forallowEmpty=true.src/input/tags-input/tags-input.component.ts (2)
37-40: LGTM! Clean interface definition for internal tag state.The
TagIteminterface clearly encapsulates the tag value and its inputting state, providing a clean abstraction for the component's internal model.
53-54: LGTM! Standalone component migration with proper imports.The component is correctly converted to standalone with
TagComponentimported. The extended generic typeCommonFormControl<string[], TagItem[]>properly separates external value type from internal model type.Also applies to: 63-65
| protected override valueIn(v: string[]): TagItem[] { | ||
| const tags = v || []; | ||
|
|
||
| const items = tags.map((value, index) => ({ | ||
| value, | ||
| isInputting: | ||
| this.model?.[index]?.value === value | ||
| ? this.model[index].isInputting | ||
| : false, | ||
| })); | ||
|
|
||
| if (!items.some(item => item.isInputting)) { | ||
| this.clearInput(); | ||
| } | ||
|
|
||
| return this.sortByReadonly(items); | ||
| } |
There was a problem hiding this comment.
Potential issue with valueIn state mapping.
The logic at lines 278-280 attempts to preserve isInputting state by matching against this.model[index], but this.model may have a different length or order than the incoming v array (e.g., after sorting, removal, or external value changes). This could lead to incorrect state preservation.
🔧 Consider matching by value instead of index
protected override valueIn(v: string[]): TagItem[] {
const tags = v || [];
+ const modelMap = new Map(this.model?.map(item => [item.value, item]) ?? []);
const items = tags.map((value, index) => ({
value,
- isInputting:
- this.model?.[index]?.value === value
- ? this.model[index].isInputting
- : false,
+ isInputting: modelMap.get(value)?.isInputting ?? false,
}));
if (!items.some(item => item.isInputting)) {
this.clearInput();
}
return this.sortByReadonly(items);
}📝 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.
| protected override valueIn(v: string[]): TagItem[] { | |
| const tags = v || []; | |
| const items = tags.map((value, index) => ({ | |
| value, | |
| isInputting: | |
| this.model?.[index]?.value === value | |
| ? this.model[index].isInputting | |
| : false, | |
| })); | |
| if (!items.some(item => item.isInputting)) { | |
| this.clearInput(); | |
| } | |
| return this.sortByReadonly(items); | |
| } | |
| protected override valueIn(v: string[]): TagItem[] { | |
| const tags = v || []; | |
| const modelMap = new Map(this.model?.map(item => [item.value, item]) ?? []); | |
| const items = tags.map((value, index) => ({ | |
| value, | |
| isInputting: modelMap.get(value)?.isInputting ?? false, | |
| })); | |
| if (!items.some(item => item.isInputting)) { | |
| this.clearInput(); | |
| } | |
| return this.sortByReadonly(items); | |
| } |
🤖 Prompt for AI Agents
In @src/input/tags-input/tags-input.component.ts around lines 272 - 288, In
valueIn, the current index-based preservation of isInputting can misalign when
this.model differs in length/order; update valueIn to map incoming v to TagItem
by finding the corresponding entry in this.model by value (e.g., locate
this.model.find(m => m.value === value)) and use its isInputting if found,
otherwise false; after building items keep the existing check that calls
clearInput() when no item.isInputting, and return the result through
sortByReadonly—refer to valueIn, this.model, clearInput, sortByReadonly, and
TagItem when making the change.
| } else if (this.inputControl.pending) { | ||
| // PENDING 后只会变为 VALID 或 INVALID 的决议状态 | ||
| this.inputControl.statusChanges.pipe(take(1)).subscribe(_ => { | ||
| this.syncControlStatus(); | ||
| if (this.inputControl.valid) { | ||
| this.emitValue(this.model.concat(value)); | ||
| this.confirmInputtingItem(); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing handling for async validator failure.
When the async validator resolves to invalid, the inputting item remains in the model indefinitely. Consider adding an else branch to handle the invalid case by removing the inputting item.
🐛 Proposed fix
this.inputControl.statusChanges.pipe(take(1)).subscribe(_ => {
this.syncControlStatus();
if (this.inputControl.valid) {
this.confirmInputtingItem();
+ } else {
+ this.removeInputtingItem();
}
});📝 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.
| } else if (this.inputControl.pending) { | |
| // PENDING 后只会变为 VALID 或 INVALID 的决议状态 | |
| this.inputControl.statusChanges.pipe(take(1)).subscribe(_ => { | |
| this.syncControlStatus(); | |
| if (this.inputControl.valid) { | |
| this.emitValue(this.model.concat(value)); | |
| this.confirmInputtingItem(); | |
| } | |
| }); | |
| } | |
| } else if (this.inputControl.pending) { | |
| this.inputControl.statusChanges.pipe(take(1)).subscribe(_ => { | |
| this.syncControlStatus(); | |
| if (this.inputControl.valid) { | |
| this.confirmInputtingItem(); | |
| } else { | |
| this.removeInputtingItem(); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In @src/input/tags-input/tags-input.component.ts around lines 343 - 350, The
code watches this.inputControl.pending and then on statusChanges calls
syncControlStatus() and confirmInputtingItem() if valid, but it doesn’t handle
the case when the async validator resolves to invalid; update the subscription
in the pending branch to check this.inputControl.valid and call
confirmInputtingItem() as now, and add an else branch that removes the
transient/inputting item from the model (the same model mutated by
confirmInputtingItem()) — you can call the existing removal logic or a helper
(e.g., removeInputtingItem or the code path that cancels input) after
syncControlStatus() when this.inputControl.valid is false to ensure the
inputting item is not left indefinitely.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/release-prod.yml:
- Around line 10-12: The workflow currently sets permissions to "contents: read"
which prevents the job from doing git push; update the permissions block so
"contents" is set to "write" (keep id-token as write) so the step that runs git
push --follow-tags origin "$PUBLISH_BRANCH" can succeed; locate the permissions
block in the release-prod.yml and change the contents permission to write.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/release-beta.yml.github/workflows/release-prod.yml.github/workflows/release.ymlscripts/release.sh
🔇 Additional comments (6)
scripts/release.sh (1)
12-13: LGTM!The switch from
npm setto direct.npmrcwrites is a reliable approach for CI environments and works well with npm provenance. The token interpolation and registry configuration are correct..github/workflows/release.yml (2)
8-11: LGTM!The permissions block is correctly configured.
id-token: writeis required for npm provenance attestations, andcontents: write/pull-requests: writeare needed for the changesets action to create release PRs and push tags.
45-46: LGTM!
NODE_AUTH_TOKENworks withactions/setup-node's registry configuration, andNPM_CONFIG_PROVENANCE: trueenables provenance attestations. Both are correctly set alongsideNPM_TOKEN(used by the release script)..github/workflows/release-beta.yml (2)
7-9: LGTM!Permissions are appropriately scoped:
contents: readfor checkout andid-token: writefor npm provenance. Beta releases don't need write access to contents since there's no git push operation.
29-49: LGTM!The inline release flow is clean and consistent with the other workflows. The
--provenance --access publicflags are correctly applied for npm attestation support.One consideration: this workflow triggers on all PRs to master. If fork PRs shouldn't publish beta releases, ensure
secrets.NPM_TOKENis not available to fork PRs (which is the default GitHub behavior for secrets)..github/workflows/release-prod.yml (1)
37-57: Build and release flow looks good otherwise.The inline release logic correctly handles version injection, conditional git push, and npm publish with provenance. The
NODE_AUTH_TOKENis properly configured for registry authentication.
fengtianze
force-pushed
the
feat/optimize-tags-input
branch
from
00270c9 to
e87e42a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/release-prod.yml (1)
37-38: Remove the redundantyarn buildfrom either the workflow orrelease.sh.The Build step (line 38) is executed before the Release step (line 41) calls
scripts/release.sh, which also runsyarn build(line 26). This causes the project to be built twice during prod releases.Either:
- Remove the build call from
release.shif the workflow will always run it beforehand, or- Remove the explicit Build step from the workflow if
release.shshould remain self-contained.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/release-beta.yml.github/workflows/release-prod.yml.github/workflows/release.ymlpackage.jsonscripts/npm-tag.jsscripts/release.sh
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/release-beta.yml
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/npm-tag.js (2)
scripts/publish-version.js (1)
version(5-5)scripts/publish-branch.js (1)
version(3-3)
🔇 Additional comments (10)
scripts/npm-tag.js (1)
3-10: Good defensive handling for undefined version.The fallback logic prevents crashes when no version is provided. One minor note: the error message mentions only
PUBLISH_VERSIONbut the value could also come fromprocess.argv[2]. Consider a more generic message like "Version is undefined" for clarity.scripts/release.sh (3)
5-5: LGTM!Correctly passes
$PUBLISH_VERSIONto the npm-tag script, aligning with the updated script that now prioritizes CLI arguments.
12-13: LGTM on the auth configuration approach.Writing the token directly to
~/.npmrcis a valid approach for CI environments. The ephemeral nature of CI runners mitigates persistence concerns.
32-32: Good addition of--provenanceand--access public.The
--provenanceflag enables npm provenance attestation (requiresid-token: writepermission, which is correctly configured in the workflows). The--access publicflag ensures scoped packages are published publicly..github/workflows/release.yml (3)
8-11: Permissions correctly configured for provenance and changesets.
id-token: write— Required for npm provenance attestationcontents: write— Required for changesets to push commits/tagspull-requests: write— Required for changesets to create release PRs
29-29: Good addition ofregistry-url.Setting the registry URL in
actions/setup-nodeensures proper authentication configuration and is required when usingNODE_AUTH_TOKEN.
45-46: LGTM on the token and provenance configuration.
NODE_AUTH_TOKENis the expected environment variable foractions/setup-nodeauthentication.NPM_CONFIG_PROVENANCE: trueenables provenance attestation at the npm config level, complementing the--provenanceflag in release.sh..github/workflows/release-prod.yml (3)
10-12: Permissions appropriately scoped.Using
contents: read(instead ofwritein release.yml) is correct here since the prod release workflow doesn't need to push commits or create PRs.
27-27: LGTM on registry-url addition.Consistent with the other workflow files.
44-45: LGTM on authentication setup.Both
NODE_AUTH_TOKENandNPM_TOKENare set, ensuring compatibility with bothactions/setup-nodeauthentication and the manual.npmrcconfiguration inrelease.sh.
fengtianze
force-pushed
the
feat/optimize-tags-input
branch
from
e87e42a to
0b1cad1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/release.sh (1)
7-11: Error message inconsistent with the updated condition.The condition now checks for both
NODE_AUTH_TOKENandNPM_TOKEN, but the error message only mentionsNPM_TOKEN. Consider updating the message for clarity.📝 Suggested improvement
if [ "$NODE_AUTH_TOKEN" = "" ] && [ "$NPM_TOKEN" = "" ]; then - echo "NPM_TOKEN is not available on PR from forked repository!" + echo "NODE_AUTH_TOKEN/NPM_TOKEN is not available on PR from forked repository!" echo "If you're a member of Alauda, just checkout a new branch instead." exit 0 fi
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/release-beta.yml.github/workflows/release-prod.yml.github/workflows/release.ymlscripts/release.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/release.yml
- .github/workflows/release-beta.yml
🔇 Additional comments (4)
.github/workflows/release-prod.yml (2)
13-15: LGTM! Correct permissions for npm provenance.The
id-token: writepermission is required for OIDC-based npm provenance attestation, andcontents: writeis needed for pushing tags. This aligns with the--provenanceflag added to the publish command inscripts/release.sh.
26-26: LGTM! Proper npm authentication setup.Using
registry-urlwithactions/setup-nodeautomatically configures.npmrcto useNODE_AUTH_TOKENfor authentication, which is the recommended pattern for npm publishing in GitHub Actions.Also applies to: 40-40
scripts/release.sh (2)
13-15: Good safety guard for latest tag.Preventing accidental publishing to the
latesttag via this script is a sensible safeguard.
29-29: LGTM! Provenance and public access flags.The
--provenanceflag enables npm package provenance attestation (requires npm 9.5.0+), and--access publicis required for publishing scoped packages publicly. Both are appropriate additions for this scoped package (@alauda/ui).
fengtianze
force-pushed
the
feat/optimize-tags-input
branch
from
0b1cad1 to
e3a91a4
Compare
fengtianze
force-pushed
the
feat/optimize-tags-input
branch
from
e3a91a4 to
a570d99
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @package.json:
- Around line 5-8: The repository.url in package.json is incorrect (currently
"https://github.com/alauda/alauda-ui.git") and must match this repo to satisfy
the npm provenance check; update the "repository" -> "url" value to the correct
GitHub URL for this project (e.g., "https://github.com/alauda/ui.git") so the
package.json repository metadata points to the actual repository.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/release-beta.yml.github/workflows/release-prod.yml.github/workflows/release.ymlpackage.jsonscripts/release.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/release.yml
- .github/workflows/release-prod.yml
🧰 Additional context used
🪛 GitHub Actions: Release beta
package.json
[error] 1-1: npm publish failed with E422 Unprocessable Entity. Provenance check failed: repository.url in package.json (git+https://github.com/alauda/alauda-ui.git) does not match expected provenance repository URL (https://github.com/alauda/ui). Please fix repository.url to match provenance or update provenance configuration.
🔇 Additional comments (6)
.github/workflows/release-beta.yml (3)
10-12: LGTM!The
id-token: writepermission is correctly added to enable OIDC token generation required for npm provenance. Combined withcontents: read, this follows the principle of least privilege.
18-23: LGTM!Adding
registry-urlis required foractions/setup-nodeto properly configure npm authentication. This works in conjunction withNODE_AUTH_TOKENfor provenance-enabled publishing.
28-32: LGTM!Using
NODE_AUTH_TOKENis the correct approach whenregistry-urlis configured inactions/setup-node. The action automatically sets up.npmrcto use this environment variable for authentication.scripts/release.sh (3)
7-11: LGTM!Good backward compatibility by accepting either
NODE_AUTH_TOKEN(used byactions/setup-nodewithregistry-url) or the legacyNPM_TOKEN. The graceful exit with code 0 for forked PRs is appropriate to allow CI checks to pass without publishing.
13-21: LGTM!Explicitly blocking
latesttag publication via this script is a good safeguard to prevent accidental production releases.
29-29: LGTM!The
--provenanceflag enables npm provenance for supply chain security, and--access publicis required for scoped packages. These flags align with the workflow permission changes (id-token: write).
| "repository": { | ||
| "type": "git", | ||
| "url": "https://github.com/alauda/alauda-ui.git" | ||
| }, |
There was a problem hiding this comment.
Repository URL mismatch causing pipeline failure.
The repository.url points to alauda/alauda-ui but this PR is in alauda/ui. This mismatch is causing the npm provenance check to fail with E422, as indicated by the pipeline error.
🔧 Proposed fix
"repository": {
"type": "git",
- "url": "https://github.com/alauda/alauda-ui.git"
+ "url": "https://github.com/alauda/ui.git"
},📝 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.
| "repository": { | |
| "type": "git", | |
| "url": "https://github.com/alauda/alauda-ui.git" | |
| }, | |
| "repository": { | |
| "type": "git", | |
| "url": "https://github.com/alauda/ui.git" | |
| }, |
🤖 Prompt for AI Agents
In @package.json around lines 5 - 8, The repository.url in package.json is
incorrect (currently "https://github.com/alauda/alauda-ui.git") and must match
this repo to satisfy the npm provenance check; update the "repository" -> "url"
value to the correct GitHub URL for this project (e.g.,
"https://github.com/alauda/ui.git") so the package.json repository metadata
points to the actual repository.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @scripts/release.sh:
- Around line 7-11: The error message in the conditional that checks [
"$NODE_AUTH_TOKEN" = "" ] && [ "$NPM_TOKEN" = "" ] only mentions NPM_TOKEN;
update the echo(s) in that if block to clearly state that neither
NODE_AUTH_TOKEN nor NPM_TOKEN is available (for example: "Neither
NODE_AUTH_TOKEN nor NPM_TOKEN is available on PRs from forked repositories") and
keep the existing guidance line; modify the echo lines in the same if block so
the message reflects both token options.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/release-beta.yml.github/workflows/release-prod.yml.github/workflows/release.ymlpackage.jsonscripts/release.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/release-beta.yml
- .github/workflows/release-prod.yml
🔇 Additional comments (5)
.github/workflows/release.yml (3)
12-14: LGTM! Proper permissions for npm provenance.The
id-token: writepermission enables OIDC token generation for npm provenance attestation, which improves supply chain security. Combined withcontents: writefor release operations, these are the minimal permissions needed.
22-27: LGTM! Registry URL correctly configured.Setting
registry-urlis required foractions/setup-nodeto generate the.npmrcfile that referencesNODE_AUTH_TOKENfor authentication.
32-42: LGTM! Correct authentication setup for npm publishing.
NODE_AUTH_TOKENis the environment variable thatactions/setup-nodeexpects whenregistry-urlis configured. This aligns with the standard GitHub Actions pattern for npm authentication.package.json (1)
5-8: LGTM!The structured repository format is cleaner and more explicit. The URL correctly reflects the current repository location.
scripts/release.sh (1)
29-29: Good addition of--provenancefor supply chain security.The
--provenanceflag links the published package to its source repository and build environment, improving supply chain transparency. The script is correctly configured to run only from GitHub Actions workflows with the requiredid-token: writepermission, ensuring the flag operates as intended.
| if [ "$NODE_AUTH_TOKEN" = "" ] && [ "$NPM_TOKEN" = "" ]; then | ||
| echo "NPM_TOKEN is not available on PR from forked repository!" | ||
| echo "If you're a member of Alauda, just checkout a new branch instead." | ||
| exit 0 | ||
| else | ||
| npm set //registry.npmjs.org/:_authToken "$NPM_TOKEN" | ||
| fi |
There was a problem hiding this comment.
Error message should mention both token options.
The condition now checks for both NODE_AUTH_TOKEN and NPM_TOKEN, but the error message only references NPM_TOKEN. This could confuse contributors who set NODE_AUTH_TOKEN but still see the error.
Proposed fix
if [ "$NODE_AUTH_TOKEN" = "" ] && [ "$NPM_TOKEN" = "" ]; then
- echo "NPM_TOKEN is not available on PR from forked repository!"
+ echo "NODE_AUTH_TOKEN or NPM_TOKEN is not available on PR from forked repository!"
echo "If you're a member of Alauda, just checkout a new branch instead."
exit 0
fi📝 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.
| if [ "$NODE_AUTH_TOKEN" = "" ] && [ "$NPM_TOKEN" = "" ]; then | |
| echo "NPM_TOKEN is not available on PR from forked repository!" | |
| echo "If you're a member of Alauda, just checkout a new branch instead." | |
| exit 0 | |
| else | |
| npm set //registry.npmjs.org/:_authToken "$NPM_TOKEN" | |
| fi | |
| if [ "$NODE_AUTH_TOKEN" = "" ] && [ "$NPM_TOKEN" = "" ]; then | |
| echo "NODE_AUTH_TOKEN or NPM_TOKEN is not available on PR from forked repository!" | |
| echo "If you're a member of Alauda, just checkout a new branch instead." | |
| exit 0 | |
| fi |
🤖 Prompt for AI Agents
In @scripts/release.sh around lines 7 - 11, The error message in the conditional
that checks [ "$NODE_AUTH_TOKEN" = "" ] && [ "$NPM_TOKEN" = "" ] only mentions
NPM_TOKEN; update the echo(s) in that if block to clearly state that neither
NODE_AUTH_TOKEN nor NPM_TOKEN is available (for example: "Neither
NODE_AUTH_TOKEN nor NPM_TOKEN is available on PRs from forked repositories") and
keep the existing guidance line; modify the echo lines in the same if block so
the message reflects both token options.
Summary by CodeRabbit
Refactor
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.