Improvement/add unused vars rule#109
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapp/controller/keyConfigs/detail/Detail.controller.ts (1)
302-334:⚠️ Potential issue | 🟠 MajorHandle workflow creation failures explicitly.
Line 302 fire-and-forgets an async call, while
createWorkflowForSystemConnection(Line 326) has nocatch. A rejected workflow creation can become an unhandled rejection and skip user-facing error feedback.Suggested fix
private async createWorkflowForSystemConnection(workflowParams: WorkflowParams): Promise<void> { try { await this.workflowComponent?.createWorkflow(workflowParams); } + catch (error) { + console.error(error); + showErrorMessage(error as AxiosError, this.getText('errorConnectingSystems')); + } finally { await this.getKeyConfigData(); this.getView()?.setBusy(false); this.onConnectSystemsCancelPress(); } }
🧹 Nitpick comments (1)
webapp/component/HyokCryptoRoleAdd.ts (1)
32-37: RenamerootCryptoCAto a profile-ARN-aligned field name.The field currently stores profile ARN data, not a root CA. Renaming (for example,
profileCryptoARN) will make model/payload mapping clearer and reduce future mistakes.Also applies to: 192-197, 285-289
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53e27168-8d4f-4744-9fd0-3ac6fc9cbcad
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
.husky/pre-commit.husky/pre-pusheslint.config.mjspackage.jsontsconfig.jsonwebapp/common/Formatters.tswebapp/component/HelpInfoPopover.tswebapp/component/HyokCryptoRoleAdd.tswebapp/controller/App.controller.tswebapp/controller/BaseController.tswebapp/controller/groups/Groups.controller.tswebapp/controller/keyConfigs/KeyConfigs.controller.tswebapp/controller/keyConfigs/detail/Detail.controller.tswebapp/controller/keyConfigs/detail/DetailPanel.controller.tswebapp/controller/systems/Layout.controller.tswebapp/controller/systems/Systems.controller.tswebapp/controller/tasks/Layout.controller.tswebapp/controller/tasks/Tasks.controller.tswebapp/resources/fragments/common/HYOKAddCryptoCertsDialog.fragment.xmlwebapp/resources/fragments/common/HYOKKeyCreationWizard.fragment.xmlwebapp/services/Api.service.tswebapp/view/keyConfigs/detail/DetailPanel.view.xml
💤 Files with no reviewable changes (5)
- webapp/controller/systems/Layout.controller.ts
- webapp/controller/tasks/Layout.controller.ts
- webapp/controller/App.controller.ts
- webapp/component/HelpInfoPopover.ts
- webapp/services/Api.service.ts
| this.addCryptoModel.setData({ | ||
| allCryptoCerts: allCryptoCerts, | ||
| availableCryptoCertsSelectionList: availableCerts, | ||
| newCryptoCertGroups: [], | ||
| allowAddMoreCryptoCert: availableCerts.length > 0, | ||
| hasCommittedCerts: false, | ||
| showAllCertsAssignedMessage: false, | ||
| certSelected: false, |
There was a problem hiding this comment.
Fix empty-state message condition for “all certs assigned”.
The message stays hidden when there are zero available certs and zero committed certs, which leaves a blank dialog state.
Suggested fix
this.addCryptoModel.setData({
...
- showAllCertsAssignedMessage: false,
+ showAllCertsAssignedMessage: availableCerts.length === 0,
...
});
private updateComputedFlags(): void {
const groups = this.addCryptoModel.getProperty('/newCryptoCertGroups') as CommittedCertEntry[];
const hasCommitted = groups && groups.length > 0;
const allowMore = this.addCryptoModel.getProperty('/allowAddMoreCryptoCert') as boolean;
this.addCryptoModel.setProperty('/hasCommittedCerts', hasCommitted);
- this.addCryptoModel.setProperty('/showAllCertsAssignedMessage', !allowMore && hasCommitted);
+ this.addCryptoModel.setProperty('/showAllCertsAssignedMessage', !allowMore);
}Also applies to: 295-301
| public onCommitCurrentCert(): void { | ||
| const currentCert = this.addCryptoModel.getProperty('/currentCert') as AWScertificates; | ||
| const trustAnchorARN = this.addCryptoModel.getProperty('/currentTrustAnchorARN') as string; | ||
| const roleARN = this.addCryptoModel.getProperty('/currentRoleARN') as string; | ||
| const profileARN = this.addCryptoModel.getProperty('/currentProfileARN') as string; | ||
|
|
||
| if (!currentCert) { | ||
| return; | ||
| } | ||
|
|
||
| const newEntry: CommittedCertEntry = { | ||
| selectedCertName: currentCert.name, | ||
| trustAnchorCryptoARN: trustAnchorARN, | ||
| roleCryptoARN: roleARN, | ||
| rootCryptoCA: profileARN | ||
| }; |
There was a problem hiding this comment.
Block commit when ARN fields are empty.
onCommitCurrentCert allows committing with blank trustAnchorARN, roleARN, or profileARN. That can send incomplete crypto access details on save.
Suggested fix
public onCommitCurrentCert(): void {
const currentCert = this.addCryptoModel.getProperty('/currentCert') as AWScertificates;
const trustAnchorARN = this.addCryptoModel.getProperty('/currentTrustAnchorARN') as string;
const roleARN = this.addCryptoModel.getProperty('/currentRoleARN') as string;
const profileARN = this.addCryptoModel.getProperty('/currentProfileARN') as string;
if (!currentCert) {
return;
}
+ if (!trustAnchorARN.trim() || !roleARN.trim() || !profileARN.trim()) {
+ MessageBox.error(this.parentController.getText('errorGeneric'));
+ return;
+ }
const newEntry: CommittedCertEntry = {📝 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.
| public onCommitCurrentCert(): void { | |
| const currentCert = this.addCryptoModel.getProperty('/currentCert') as AWScertificates; | |
| const trustAnchorARN = this.addCryptoModel.getProperty('/currentTrustAnchorARN') as string; | |
| const roleARN = this.addCryptoModel.getProperty('/currentRoleARN') as string; | |
| const profileARN = this.addCryptoModel.getProperty('/currentProfileARN') as string; | |
| if (!currentCert) { | |
| return; | |
| } | |
| const newEntry: CommittedCertEntry = { | |
| selectedCertName: currentCert.name, | |
| trustAnchorCryptoARN: trustAnchorARN, | |
| roleCryptoARN: roleARN, | |
| rootCryptoCA: profileARN | |
| }; | |
| public onCommitCurrentCert(): void { | |
| const currentCert = this.addCryptoModel.getProperty('/currentCert') as AWScertificates; | |
| const trustAnchorARN = this.addCryptoModel.getProperty('/currentTrustAnchorARN') as string; | |
| const roleARN = this.addCryptoModel.getProperty('/currentRoleARN') as string; | |
| const profileARN = this.addCryptoModel.getProperty('/currentProfileARN') as string; | |
| if (!currentCert) { | |
| return; | |
| } | |
| if (!trustAnchorARN.trim() || !roleARN.trim() || !profileARN.trim()) { | |
| MessageBox.error(this.parentController.getText('errorGeneric')); | |
| return; | |
| } | |
| const newEntry: CommittedCertEntry = { | |
| selectedCertName: currentCert.name, | |
| trustAnchorCryptoARN: trustAnchorARN, | |
| roleCryptoARN: roleARN, | |
| rootCryptoCA: profileARN | |
| }; |
| public onAddHYOKCryptoDetailsPress(): void { | ||
| const selectedKey = this.twoWayModel.getProperty('/selectedKey') as Key; | ||
| const existingCrypto = selectedKey?.accessDetails?.crypto; | ||
| this.addCryptoComponent?.openAddCryptoCertsDialog( | ||
| { | ||
| keyId: this.id, | ||
| keyConfigId: this.keyConfigId ?? '', | ||
| existingCrypto: existingCrypto | ||
| }, | ||
| this, | ||
| this.api, | ||
| async () => { | ||
| await this.getKeyDetails(); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Fail fast when keyConfigId is missing.
keyConfigId is optional in the route params, but this path silently falls back to ''. If the route is malformed, the add dialog will open with an invalid config id and fail later in the flow.
Suggested fix
public onAddHYOKCryptoDetailsPress(): void {
const selectedKey = this.twoWayModel.getProperty('/selectedKey') as Key;
const existingCrypto = selectedKey?.accessDetails?.crypto;
+ if (!this.keyConfigId) {
+ console.error('Missing keyConfigId');
+ return;
+ }
this.addCryptoComponent?.openAddCryptoCertsDialog(
{
keyId: this.id,
- keyConfigId: this.keyConfigId ?? '',
+ keyConfigId: this.keyConfigId,
existingCrypto: existingCrypto
},📝 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.
| public onAddHYOKCryptoDetailsPress(): void { | |
| const selectedKey = this.twoWayModel.getProperty('/selectedKey') as Key; | |
| const existingCrypto = selectedKey?.accessDetails?.crypto; | |
| this.addCryptoComponent?.openAddCryptoCertsDialog( | |
| { | |
| keyId: this.id, | |
| keyConfigId: this.keyConfigId ?? '', | |
| existingCrypto: existingCrypto | |
| }, | |
| this, | |
| this.api, | |
| async () => { | |
| await this.getKeyDetails(); | |
| } | |
| ); | |
| public onAddHYOKCryptoDetailsPress(): void { | |
| const selectedKey = this.twoWayModel.getProperty('/selectedKey') as Key; | |
| const existingCrypto = selectedKey?.accessDetails?.crypto; | |
| if (!this.keyConfigId) { | |
| console.error('Missing keyConfigId'); | |
| return; | |
| } | |
| this.addCryptoComponent?.openAddCryptoCertsDialog( | |
| { | |
| keyId: this.id, | |
| keyConfigId: this.keyConfigId, | |
| existingCrypto: existingCrypto | |
| }, | |
| this, | |
| this.api, | |
| async () => { | |
| await this.getKeyDetails(); | |
| } | |
| ); |
| <m:HBox> | ||
| <m:HBox> | ||
| <m:Button | ||
| icon="sap-icon://edit" | ||
| icon="sap-icon://add" | ||
| type="Ghost" | ||
| text="{i18n>edit}" | ||
| press="onEditHYOKCryptoDetailsPress"> | ||
| text="{i18n>add}" | ||
| press="onAddHYOKCryptoDetailsPress"> | ||
| <m:customData> | ||
| <core:CustomData | ||
| key="testId" | ||
| value="section-editButton" | ||
| value="section-addButton" | ||
| writeToDom="true" /> | ||
| </m:customData> | ||
| </m:Button> |
There was a problem hiding this comment.
Guard the new add action while crypto edit mode is active.
As written, this button can open the add dialog while cryptoInEditMode is true. That dialog flow refreshes key details on save, which can overwrite unsaved edits in the current form.
Suggested fix
- <m:Button
- icon="sap-icon://add"
- type="Ghost"
- text="{i18n>add}"
- press="onAddHYOKCryptoDetailsPress">
+ <m:Button
+ icon="sap-icon://add"
+ type="Ghost"
+ text="{i18n>add}"
+ visible="{= !${oneWay>/cryptoInEditMode}}"
+ press="onAddHYOKCryptoDetailsPress">1 participant
What this PR does / why we need it:
Developers can currently push code containing unused variables, unused class members, and other lint errors to the remote repository without any local enforcement. This results in dead code accumulating (e.g.,
hasHandledFirstAuthErrorandisLoginInProgressinApi.service.tswere declared but never used) and unnecessary CI pipeline failures.This ticket addresses two gaps:
Summary by CodeRabbit
New Features
Chores