Skip to content

Revert "Revert "Production pr ""#443

Open
Chandan-walker wants to merge 1 commit intoproductionfrom
revert-442-revert-441-main
Open

Revert "Revert "Production pr ""#443
Chandan-walker wants to merge 1 commit intoproductionfrom
revert-442-revert-441-main

Conversation

@Chandan-walker
Copy link
Collaborator

Reverts #442

Copy link

@windsurf-bot windsurf-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other comments (2)
  • apps/proxy/src/app/features/create-feature/create-feature.component.ts (755-761) There's a commented out UI preferences block that should either be removed or uncommented if it's needed. Commented code can lead to confusion during maintenance.
  • apps/proxy/src/styles.scss (109-113) This PR is adding commented-out CSS code. If this styling is needed, it should be uncommented and properly implemented. If it's not needed yet but will be used in the future, consider adding a TODO comment explaining when it will be used. Otherwise, it's better to remove commented code to keep the codebase clean.

💡 To request another review, post a new comment with "/windsurf-review".

Comment on lines +582 to +583
// this.featureForm.get('brandingDetails.logo_url')?.setValue(url);
this.logoUrl = url;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a commented out line that would set the logo_url in the form, but instead the value is stored in this.logoUrl. This creates an inconsistency between the form state and the component state.

Suggested change
// this.featureForm.get('brandingDetails.logo_url')?.setValue(url);
this.logoUrl = url;
this.featureForm.get('brandingDetails.logo_url')?.setValue(url);
this.logoUrl = url;

Comment on lines +141 to +145
(click)="
setRedirectUrlInServiceDetails();
featureForm.get('authorizationDetails').markAllAsTouched();
createFeature()
"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're calling createFeature() directly after marking fields as touched, but there's no validation check. This could submit invalid data. Consider adding a condition to only call createFeature() if the form is valid.

Validators.maxLength(60),
]),
feature_id: new FormControl<number>(null, [Validators.required]),
feature_id: new FormControl<number>(1, [Validators.required]),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feature_id FormControl is initialized with a hardcoded value of 1, which might not be appropriate for all cases. Consider initializing it as null and setting it to 1 only when needed, or document why it's always initialized to 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants