-
Notifications
You must be signed in to change notification settings - Fork 92
PackageDialog: centralized state management, load manifest on demand #4564
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
base: master
Are you sure you want to change the base?
Conversation
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.
Greptile Overview
Summary
This PR represents a significant architectural improvement to the PackageDialog component, replacing react-final-form with a centralized custom state management system.
Key Changes
- Centralized State Management: New
State/
directory consolidates all form state management into custom hooks with clear separation of concerns - Manifest Loading on Demand: Added intelligent manifest loading that prevents accidental package overwrites by letting users explicitly choose to load existing packages
- Improved Type Safety: Enhanced TypeScript usage throughout with better error handling and validation patterns
- Test Coverage: Added 11 new test files covering the new state management hooks and validation logic
- Code Organization: Split monolithic
PackageCreationForm.tsx
(878 lines) into focused, reusable components
Technical Improvements
- Replaced complex form library dependency with lightweight, purpose-built state management
- Added debounced package name validation with better user feedback
- Improved error handling with field-specific validation states
- Enhanced manifest loading workflow with user-controlled loading to prevent data loss
The refactoring maintains backward compatibility while providing a more maintainable and user-friendly package creation experience. The new architecture is well-tested and follows React best practices with proper separation of UI and business logic.
Confidence Score: 4/5
- This PR is safe to merge with proper testing and code review
- Score reflects comprehensive refactoring with good test coverage and clean architecture. Minor optimization opportunities exist but don't impact functionality
- Pay close attention to State/manifest.ts for potential optimization and State/State.tsx TODO comment about state splitting
Important Files Changed
File Analysis
Filename | Score | Overview |
---|---|---|
catalog/app/containers/Bucket/PackageDialog/State/State.tsx | 4/5 | Main state orchestration hook with comprehensive type safety and centralized management, minor TODO about splitting state |
catalog/app/containers/Bucket/PackageDialog/Create.tsx | 5/5 | Clean React component for package creation with proper error handling and dialog flow management |
catalog/app/containers/Bucket/PackageDialog/Copy.tsx | 5/5 | Package copying component with proper validation and simplified state management compared to old approach |
catalog/app/containers/Bucket/PackageDialog/State/manifest.ts | 4/5 | Manifest loading logic with proper state management, potential optimization opportunities for re-fetch prevention |
catalog/app/containers/Bucket/PackageDialog/State/params.ts | 5/5 | Form parameter validation with comprehensive error handling and type safety |
catalog/app/containers/Bucket/PackageDialog/State/form.ts | 5/5 | Form status management with proper validation for Web Cryptography API availability |
Sequence Diagram
sequenceDiagram
participant User
participant CreateTsx as Create.tsx
participant StateTsx as State/State.tsx
participant NameTs as State/name.ts
participant ManifestTs as State/manifest.ts
participant FormTs as State/form.ts
participant FilesTs as State/files.ts
participant API as GraphQL API
User->>CreateTsx: "Opens package creation dialog"
CreateTsx->>StateTsx: "useState(dst, src, open)"
StateTsx->>FormTs: "useFormStatus(open)"
StateTsx->>ManifestTs: "useManifestRequest(open, src)"
StateTsx->>NameTs: "useName(formStatus, dst, setDst, src)"
StateTsx->>FilesTs: "useFiles(formStatus, schema, manifest, open)"
alt Package name entered
User->>NameTs: "onChange(packageName)"
NameTs->>API: "Package existence check (debounced)"
API-->>NameTs: "Package exists response"
NameTs->>StateTsx: "Update name status"
StateTsx->>ManifestTs: "setSrc (if load existing selected)"
ManifestTs->>API: "Fetch manifest"
API-->>ManifestTs: "Return manifest data"
ManifestTs->>FilesTs: "Update files from manifest"
FilesTs->>StateTsx: "Update files state"
end
alt Submit form
User->>CreateTsx: "Click submit"
CreateTsx->>StateTsx: "create()"
StateTsx->>FormTs: "setFormStatus(submitting)"
StateTsx->>API: "Upload files + create package"
API-->>StateTsx: "Package creation response"
StateTsx->>FormTs: "setFormStatus(success/error)"
FormTs->>CreateTsx: "Update UI state"
end
Note over StateTsx: "Centralized state management replaces react-final-form"
57 files reviewed, 2 comments
… hard reset copy dialog state on successor change
Need to fix bookmarks |
Refactored PackageDialog with centralized state management and manifest loading.
State/
directorydialog.open
can acceptadd
parameter waslogicalKey
string, now it is quilt+s3 URIadd
already contains package info, no need in "next" parameter to redirect back to package?action=revisePackage
replaced bypackage/name/add?logicalKey=s3://file-url
, and temporary workaroundapp/containers/AddToPackage
was removed.MarkdownJSDoc docs for developers