-
Notifications
You must be signed in to change notification settings - Fork 1.5k
refactor: Modernize Project Structure and Refine Codebase #1812
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: main
Are you sure you want to change the base?
refactor: Modernize Project Structure and Refine Codebase #1812
Conversation
* feat: refactor main menu layout and clean up chat-related components * Some minor layout fixes * iterate over all TextParts * add navRoute in Sample * fix style in light/dark mode * change Hstack to Label for multi-lines * add .inline for navigationTitleMode
* integrate conversationkit to firebaseai * refract functioncalling logic * remove fatalError * change preview for screens * ♻️ Use ConversationKit * ♻️ Refactor error handling * ✨ Bring back pre-filled user messages * 🧹Cleanup * ⬆️ Use latest ConversationKit version * fix style check * add errordetailview for imagenexample * ✏️ Fix typo Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * ✏️ Fix typo Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * ✏️ Fix typo Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * ✏️ Fix typo Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * fix style and change ci --------- Co-authored-by: Peter Friese <[email protected]> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
… Feature
This commit introduces a significant refactoring of the iOS project, reorganizing the codebase into a more modular and maintainable structure. All feature-specific code has been moved into a new `Features` directory, while shared components (Models, Views) are now centralized under a `Shared` directory.
Key changes include:
- **Project Structure Reorganization**:
- Created `FirebaseAIExample/Features` to house individual feature modules (e.g., `Chat`, `FunctionCalling`, `GenerativeAIText`, `Grounding`, `Imagen`, `Multimodal`).
- Created `FirebaseAIExample/Shared` for common models, views, and utilities.
- Updated `FirebaseAIExample.xcodeproj/project.pbxproj` to reflect the new file paths and group structure.
- **New Feature: 'Hello world (with template)'**:
- Implemented `GenerateContentFromTemplateScreen` and `GenerateContentFromTemplateViewModel` under `Features/GenerativeAIText`.
- This feature demonstrates server-side template generation using the `apple-qs-greeting` template.
- Registered the new feature in `ContentView` and added a corresponding `Sample` entry in `Sample.swift` with the title "Hello world (with template)".
- **Bug Fixes and Best Practices**:
- **Security Fix**: Replaced insecure API key initialization in `GenerateContentFromTemplateViewModel` with the secure `FirebaseAI.firebaseAI(backend:)` factory method.
- **Compile Error Resolution**: Moved shared UI components (`ProgressOverlay.swift`, `ErrorDetailsView.swift`) into `FirebaseAIExample/Shared/Views` to resolve compile errors and ensure global accessibility.
- **Centralized BackendOption**: Extracted the `BackendOption` enum into its own file (`FirebaseAIExample/Shared/Models/BackendOption.swift`) for better code organization and reusability.
This refactoring significantly improves the project's scalability, readability, and adherence to modern Swift/SwiftUI architectural patterns.
Summary of ChangesHello @peterfriese, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a comprehensive refactoring of the iOS project, aiming to enhance its modularity, maintainability, and scalability. The changes involve a complete overhaul of the project's directory structure, the introduction of a new templated content generation feature, and a significant upgrade to the user interface by adopting the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This is a significant and well-executed refactoring of the iOS project. The move to a feature-based architecture, centralization of shared components, and adoption of ConversationKit greatly improve the project's structure, maintainability, and scalability. The new Sample-driven approach for showcasing examples is also a fantastic improvement.
I've identified a couple of critical issues, primarily a merge conflict in a script and an invalid build setting, that must be addressed. I also have a high-severity suggestion regarding dependency pinning for better build stability and a medium-severity suggestion to improve the new navigation routing logic. Overall, this is excellent work.
| requirement = { | ||
| kind = revision; | ||
| revision = 7aff8d1b31148d32c5933d75557d42f6323ee3d1; | ||
| branch = main; | ||
| kind = branch; | ||
| }; |
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.
The ConversationKit dependency is pinned to the main branch. This can lead to non-reproducible builds as the main branch can change at any time. It's recommended to pin dependencies to a specific version tag or commit hash to ensure build stability.
- requirement = {
- branch = main;
- kind = branch;
- };
| switch sample.navRoute { | ||
| case "ChatScreen": | ||
| ChatScreen(backendType: selectedBackend, sample: sample) | ||
| case "ImagenScreen": | ||
| ImagenScreen(backendType: selectedBackend, sample: sample) | ||
| case "ImagenFromTemplateScreen": | ||
| ImagenFromTemplateScreen(backendType: selectedBackend, sample: sample) | ||
| case "GenerateContentFromTemplateScreen": | ||
| GenerateContentFromTemplateScreen(backendType: selectedBackend, sample: sample) | ||
| case "MultimodalScreen": | ||
| MultimodalScreen(backendType: selectedBackend, sample: sample) | ||
| case "FunctionCallingScreen": | ||
| FunctionCallingScreen(backendType: selectedBackend, sample: sample) | ||
| case "GroundingScreen": | ||
| GroundingScreen(backendType: selectedBackend, sample: sample) | ||
| default: | ||
| EmptyView() | ||
| } |
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.
The navigation logic in destinationView(for:) relies on string matching for sample.navRoute. This is fragile and prone to typos. Consider using an enum for the navigation routes to make this more robust and type-safe.
For example:
// In Sample.swift
enum NavRoute {
case chatScreen
case imagenScreen
// ... other cases
}
// In ContentView.swift
@ViewBuilder
private func destinationView(for sample: Sample) -> some View {
switch sample.navRoute {
case .chatScreen:
ChatScreen(backendType: selectedBackend, sample: sample)
// ... other cases
}
}
This pull request modernizes the iOS quickstart project, building upon the significant restructuring work initiated by Young Hypo. The primary goal is to create a more modular, scalable, and maintainable codebase that aligns with modern Swift and SwiftUI best practices.
Initial Refactoring (by Young Hypo):
Refinements and Additions in this PR:
Features) and shared components (Shared), improving clarity and navigation.FirebaseAI.firebaseAI(backend:)factory method.BackendOption) and UI components (ProgressOverlay,ErrorDetailsView) to eliminate redundancy and fix compilation errors.GenerateContentFromTemplateScreen) to demonstrate server-side template functionality with theapple-qs-greetingtemplate. This provides a clear and minimal entry point for users.Overall, these changes make the project easier to understand, maintain, and extend, providing a better foundation for developers exploring Firebase AI features.