-
Notifications
You must be signed in to change notification settings - Fork 51
🐛 New application form has basic and source sections expanded by default #2585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Resolves: https://issues.redhat.com/browse/MTA-6003 The new application form should have the basic details and the source code sections expanded by default. The other sections should be collapsed by default. When editing an existing application, the sections that have data should be expanded by default. Signed-off-by: Scott J Dickerson <[email protected]>
WalkthroughAdds an application prop from ApplicationFormModal to ApplicationForm. ApplicationForm uses this prop to determine initial expansion states of several sections. Strengthens typing in useApplicationForm by introducing RepositoryKind and updating FormValues.kind and assetKind types. No runtime submission/validation flow changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Modal as ApplicationFormModal
participant Form as ApplicationForm
participant UI as Expandable Sections
User->>Modal: Open application form
Modal->>Form: Render with props { form, data, application }
Note right of Form: New prop: application
alt application == null (create)
Form->>UI: Init: Source Code expanded
Form->>UI: Init: Binary collapsed
Form->>UI: Init: Source Platform collapsed
Form->>UI: Init: Asset Repository collapsed
else application != null (edit)
Form->>UI: Init based on existing fields:<br/>- Source Code if kind && sourceRepository<br/>- Binary if group && artifact && version<br/>- Source Platform if sourcePlatform<br/>- Asset Repository if assetKind && assetRepository
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/src/app/pages/applications/application-form/useApplicationForm.ts (2)
20-20: Centralize repo kind literals and derive the type from a single source of truthPrevents drift between types, validation, and select options; also enables reuse elsewhere.
-type RepositoryKind = "git" | "subversion" | ""; +export const REPOSITORY_KINDS = ["", "git", "subversion"] as const; +export type RepositoryKind = (typeof REPOSITORY_KINDS)[number];Outside this hunk, consider switching Yup to the shared list to avoid duplication:
// replace: string().oneOf(["", "git", "subversion"]) kind: string().oneOf([...REPOSITORY_KINDS]), // and for assetKind: assetKind: string().oneOf([...REPOSITORY_KINDS]),
234-234: Avoid unsafe casts; coerce to RepositoryKind with a guardRemoves the need for
asand safely normalizes unexpected values.- kind: (application?.repository?.kind ?? "") as RepositoryKind, + kind: toRepositoryKind(application?.repository?.kind), ... - assetKind: (application?.assets?.kind ?? "") as RepositoryKind, + assetKind: toRepositoryKind(application?.assets?.kind),Add outside this hunk:
const toRepositoryKind = (v: unknown): RepositoryKind => v === "git" || v === "subversion" ? v : "";Also applies to: 243-243
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
client/src/app/pages/applications/application-form/application-form-modal.tsx(1 hunks)client/src/app/pages/applications/application-form/application-form.tsx(3 hunks)client/src/app/pages/applications/application-form/useApplicationForm.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
client/src/app/pages/applications/application-form/application-form-modal.tsx (1)
client/src/app/pages/applications/application-form/application-form.tsx (1)
ApplicationForm(27-450)
client/src/app/pages/applications/application-form/application-form.tsx (3)
client/src/app/pages/applications/application-form/useApplicationForm.ts (1)
useApplicationForm(76-354)client/src/app/pages/applications/application-form/useApplicationFormData.ts (1)
useApplicationFormData(38-137)client/src/app/api/models.ts (1)
Application(138-163)
⏰ 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). (2)
- GitHub Check: unit-test
- GitHub Check: build-and-upload-for-global-ci
🔇 Additional comments (5)
client/src/app/pages/applications/application-form/useApplicationForm.ts (1)
30-30: FormValues narrowing verified – All comparisons (validation schema, URL serializers),repositoryKindOptions, and payload serialization exclusively use “git” and “subversion” (plus the empty string), matchingRepositoryKind. No mismatches detected.client/src/app/pages/applications/application-form/application-form-modal.tsx (1)
68-68: Prop plumb-through looks correctPassing
applicationdown enables the new initial expansion behavior.client/src/app/pages/applications/application-form/application-form.tsx (3)
25-25: Adding Application type import is appropriate
27-31: Props updated to include application: OK
50-68: Trim form string values in initial state to avoid whitespace-only expansion.
Example—apply to all string fields:- !!values.sourceRepository + !!values.sourceRepository?.trim()Optional: recalculate expansions in a
useEffectonapplication?.id.
mguetta1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally. It looks great
|
Thanks for handling the test update @mguetta1 ! |
…ult (konveyor#2585) Resolves: https://issues.redhat.com/browse/MTA-6003 The new application form should have the basic details and the source code sections expanded by default. The other sections should be collapsed by default. When editing an existing application, the sections that have data should be expanded by default. UI tests PR: 1660 Signed-off-by: Scott J Dickerson <[email protected]>
Resolves: https://issues.redhat.com/browse/MTA-6003
The new application form should have the basic details and the source code sections expanded by default. The other sections should be collapsed by default.
When editing an existing application, the sections that have data should be expanded by default.
Note: The UI sanity tests break with this PR. The test probably needs to only click to toggle the source repo section expansion if the source repo fields are not visible.
UI tests PR: 1660
Summary by CodeRabbit
New Features
Refactor