armistace/resbuilder_app
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

My Resumes history, modern M3 redesign, authenticated exports #2

Merged
armistace merged 10 commits from feature/my-resumes-modern into master 2026-08-04 23:58:13 +10:00
Conversation 3 Commits 10 Files Changed 54 +2091 -559
Opencode commented 2026-08-04 23:50:50 +10:00
Collaborator
Copy Link

Supersedes PR #1. Adds the My Resumes history screen, a modern Material 3 redesign (drawer navigation, richer teal palette, hero surfaces), authenticated PDF/DOCX exports, and the review-fix hygiene (R8, network security config, ViewModel refactor, .idea removal, tests).

Supersedes PR #1. Adds the My Resumes history screen, a modern Material 3 redesign (drawer navigation, richer teal palette, hero surfaces), authenticated PDF/DOCX exports, and the review-fix hygiene (R8, network security config, ViewModel refactor, .idea removal, tests).
Opencode added 9 commits 2026-08-04 23:50:50 +10:00 
- colors.xml: replaced legacy palette with M3 semantic tokens
  (primary, secondary, tertiary, surface containers, outline, status)
- themes.xml + values-night/themes.xml: switched to
  Theme.Material3.DayNight.NoActionBar, mapped all M3 color roles,
  transparent system bars for edge-to-edge, M3 typography
- attrs.xml: custom semantic attrs (colorSuccess, colorWarning, colorInfo)
- strings.xml: added accessibility labels, loading/empty/error strings
- activity_login.xml: centered branding, M3 headline/body, skeleton card
- activity_main.xml: edge-to-edge CoordinatorLayout, M3 AppBar/toolbar,
  health indicator group, M3 banner, M3 TabLayout
- fragment_job_form.xml: grouped fields in outlined cards (12dp radius),
  FilledBox text inputs, TonalButtons, 16dp margins
- fragment_job_status.xml: chip-style badge, LinearProgressIndicator,
  centered card with error/try-again states
- fragment_result.xml: SuggestionChip badge, WebView fills space,
  bottom action bar in outlined card
- activity_admin.xml + item_admin_user.xml: M3 card list items,
  proper type scale hierarchy
- Updated drawables to use theme attributes

Refs DESIGN.md Material Design 3 + Amazon/Google guidelines.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
armistace commented 2026-08-04 23:50:54 +10:00
Owner
Copy Link

PR received — starting review, sit tight 🫡

PR received — starting review, sit tight :saluting_face:
armistace commented 2026-08-04 23:52:04 +10:00
Owner
Copy Link

PR Review Results

Unified Code Review Report: Project Modernization & Authenticated Exports

1. Executive Summary

This Pull Request (PR) represents a significant evolution of the application, transitioning it from a basic utility to a production-ready, authenticated platform. The update includes a comprehensive UI/UX redesign using Material 3 (M3), the implementation of a resume history feature, and the integration of Google Authentication.

While the "project hygiene" (removal of IDE metadata and addition of R8/ProGuard) is commendable, the PR introduces critical new attack vectors related to identity management and data privacy. Because the actual code patches were not provided, the review is based on architectural changes. All three review domains (Code, Security, and Infrastructure) have flagged this PR as needing changes. The primary concerns revolve around the secure storage of OAuth tokens, the prevention of IDOR vulnerabilities in file exports, and the management of API secrets.

2. Prioritized Findings

🔴 Critical Priority (Must be fixed before merge)

  • Token Storage Security: With the introduction of LoginViewModel and ApiClient, there is a high risk that OAuth tokens are stored in plain-text SharedPreferences. This must be migrated to EncryptedSharedPreferences.
  • Secret Leakage: The addition of GOOGLE_AUTH_SETUP.md and ApiClient.kt creates a risk of hardcoded Client IDs or Secrets. These must be moved to local.properties or a Secrets Gradle Plugin.
  • Broken Access Control (IDOR): The new "Authenticated Exports" feature must be verified to ensure the backend validates ownership of the resumeId before delivery, preventing users from downloading other users' resumes.

🟠 High Priority (Should be fixed before merge)

  • Cleartext Traffic: The "network security config" must be explicitly audited to ensure cleartextTrafficPermitted is set to false for all production endpoints to prevent MitM attacks.
  • Missing Test Coverage: Despite mentions of tests in the PR description, no new test files were found in the manifest. Logic-heavy components (ApiClient, MyResumesViewModel) require unit tests.
  • Environment Parity: Base URLs for APIs must be injected via buildConfigField in Gradle rather than hardcoded in ApiClient.kt.

🟡 Medium Priority (Recommended for current sprint)

  • R8/ProGuard Validation: Verify that -keep rules are specific enough to protect logic but permissive enough to avoid runtime crashes during JSON deserialization.
  • Resource Externalization: Ensure all new M3 UI strings and colors are defined in strings.xml and colors.xml to support localization and theme maintainability.
  • Dependency Injection: Transition from manual instantiation of ApiClient to a DI framework (e.g., Hilt) to improve testability and scalability.

🔵 Low Priority (Future considerations)

  • Accessibility (WCAG): Audit the new teal palette and bg_status_badge.xml for color contrast compliance.
  • Advanced Security: Implement SSL Pinning and Root Detection (Play Integrity API) given the handling of PII.

3. Domain-Specific Recommendations

💻 Code & Architecture

  • Lifecycle Management: Ensure MyResumesViewModel utilizes viewModelScope for all asynchronous network calls to prevent memory leaks.
  • Error Handling: Implement a Result wrapper or robust try-catch blocks in ApiClient.kt to handle 4xx/5xx HTTP errors without crashing the app.
  • Naming Conventions: Maintain strict adherence to ic_ for icons and bg_ for backgrounds in the new Material 3 asset library.

🛡️ Security

  • Auth Flow: Implement a graceful session expiration and forced logout mechanism for when Google OAuth tokens expire.
  • Input Sanitization: Sanitize all user input in JobFormFragment and MyResumesFragment before transmission to prevent injection attacks.
  • Audit: Conduct a line-by-line review of ApiClient.kt specifically looking for sensitive data logging (Log.d) that might leak tokens in production.

⚙️ Infrastructure & DevOps

  • Git Hygiene: Explicitly add .idea/ to the global .gitignore to ensure the recently deleted files do not reappear.
  • CI/CD Integration: Add a GitHub Action/GitLab CI step to automatically run ./gradlew lint and ./gradlew test on every PR to enforce the described hygiene.
  • Secret Management: Move away from .md setup guides toward a secure secret management tool (e.g., GitHub Secrets) for CI/CD pipelines.

4. Positive Aspects of the PR

  • Modernization: The transition to Material 3 significantly improves the app's visual appeal and aligns it with current Android standards.
  • Professionalism: The removal of .idea folders and the addition of ProGuard rules demonstrate a strong commitment to production-grade software engineering.
  • Architectural Intent: The shift toward MVVM with MyResumesViewModel shows a healthy separation of concerns.
  • Security Proactivity: The intent to implement a network security configuration and authenticated exports shows the team is thinking about security early in the feature lifecycle.

5. Overall Assessment & Recommendation

Final Rating: $\text{Significant Changes Needed}$

Reasoning:
While the visual and structural improvements are excellent, the PR introduces critical security-sensitive logic (Authentication and PII Export) without providing the corresponding implementation code for audit. The intersection of "Authenticated Exports" and "Network Security Config" creates a high-risk profile regarding data leakage and unauthorized access.

Closing Requirement for Approval:
The developer must provide the actual code patches for ApiClient.kt, LoginViewModel.kt, and network_security_config.xml. Approval is contingent upon verifying encrypted token storage, absence of hardcoded secrets, and proven unit test coverage for the new business logic.

## PR Review Results # Unified Code Review Report: Project Modernization & Authenticated Exports ## 1. Executive Summary This Pull Request (PR) represents a significant evolution of the application, transitioning it from a basic utility to a production-ready, authenticated platform. The update includes a comprehensive UI/UX redesign using Material 3 (M3), the implementation of a resume history feature, and the integration of Google Authentication. While the "project hygiene" (removal of IDE metadata and addition of R8/ProGuard) is commendable, the PR introduces critical new attack vectors related to identity management and data privacy. Because the actual code patches were not provided, the review is based on architectural changes. **All three review domains (Code, Security, and Infrastructure) have flagged this PR as needing changes.** The primary concerns revolve around the secure storage of OAuth tokens, the prevention of IDOR vulnerabilities in file exports, and the management of API secrets. --- ## 2. Prioritized Findings ### 🔴 Critical Priority (Must be fixed before merge) * **Token Storage Security:** With the introduction of `LoginViewModel` and `ApiClient`, there is a high risk that OAuth tokens are stored in plain-text `SharedPreferences`. This must be migrated to **EncryptedSharedPreferences**. * **Secret Leakage:** The addition of `GOOGLE_AUTH_SETUP.md` and `ApiClient.kt` creates a risk of hardcoded Client IDs or Secrets. These must be moved to `local.properties` or a Secrets Gradle Plugin. * **Broken Access Control (IDOR):** The new "Authenticated Exports" feature must be verified to ensure the backend validates ownership of the `resumeId` before delivery, preventing users from downloading other users' resumes. ### 🟠 High Priority (Should be fixed before merge) * **Cleartext Traffic:** The "network security config" must be explicitly audited to ensure `cleartextTrafficPermitted` is set to `false` for all production endpoints to prevent MitM attacks. * **Missing Test Coverage:** Despite mentions of tests in the PR description, no new test files were found in the manifest. Logic-heavy components (`ApiClient`, `MyResumesViewModel`) require unit tests. * **Environment Parity:** Base URLs for APIs must be injected via `buildConfigField` in Gradle rather than hardcoded in `ApiClient.kt`. ### 🟡 Medium Priority (Recommended for current sprint) * **R8/ProGuard Validation:** Verify that `-keep` rules are specific enough to protect logic but permissive enough to avoid runtime crashes during JSON deserialization. * **Resource Externalization:** Ensure all new M3 UI strings and colors are defined in `strings.xml` and `colors.xml` to support localization and theme maintainability. * **Dependency Injection:** Transition from manual instantiation of `ApiClient` to a DI framework (e.g., Hilt) to improve testability and scalability. ### 🔵 Low Priority (Future considerations) * **Accessibility (WCAG):** Audit the new teal palette and `bg_status_badge.xml` for color contrast compliance. * **Advanced Security:** Implement SSL Pinning and Root Detection (Play Integrity API) given the handling of PII. --- ## 3. Domain-Specific Recommendations ### 💻 Code & Architecture * **Lifecycle Management:** Ensure `MyResumesViewModel` utilizes `viewModelScope` for all asynchronous network calls to prevent memory leaks. * **Error Handling:** Implement a `Result` wrapper or robust `try-catch` blocks in `ApiClient.kt` to handle 4xx/5xx HTTP errors without crashing the app. * **Naming Conventions:** Maintain strict adherence to `ic_` for icons and `bg_` for backgrounds in the new Material 3 asset library. ### 🛡️ Security * **Auth Flow:** Implement a graceful session expiration and forced logout mechanism for when Google OAuth tokens expire. * **Input Sanitization:** Sanitize all user input in `JobFormFragment` and `MyResumesFragment` before transmission to prevent injection attacks. * **Audit:** Conduct a line-by-line review of `ApiClient.kt` specifically looking for sensitive data logging (`Log.d`) that might leak tokens in production. ### ⚙️ Infrastructure & DevOps * **Git Hygiene:** Explicitly add `.idea/` to the global `.gitignore` to ensure the recently deleted files do not reappear. * **CI/CD Integration:** Add a GitHub Action/GitLab CI step to automatically run `./gradlew lint` and `./gradlew test` on every PR to enforce the described hygiene. * **Secret Management:** Move away from `.md` setup guides toward a secure secret management tool (e.g., GitHub Secrets) for CI/CD pipelines. --- ## 4. Positive Aspects of the PR * **Modernization:** The transition to Material 3 significantly improves the app's visual appeal and aligns it with current Android standards. * **Professionalism:** The removal of `.idea` folders and the addition of ProGuard rules demonstrate a strong commitment to production-grade software engineering. * **Architectural Intent:** The shift toward MVVM with `MyResumesViewModel` shows a healthy separation of concerns. * **Security Proactivity:** The intent to implement a network security configuration and authenticated exports shows the team is thinking about security early in the feature lifecycle. --- ## 5. Overall Assessment & Recommendation **Final Rating: $\text{Significant Changes Needed}$** **Reasoning:** While the visual and structural improvements are excellent, the PR introduces critical security-sensitive logic (Authentication and PII Export) without providing the corresponding implementation code for audit. The intersection of "Authenticated Exports" and "Network Security Config" creates a high-risk profile regarding data leakage and unauthorized access. **Closing Requirement for Approval:** The developer must provide the actual code patches for `ApiClient.kt`, `LoginViewModel.kt`, and `network_security_config.xml`. Approval is contingent upon verifying **encrypted token storage**, **absence of hardcoded secrets**, and **proven unit test coverage** for the new business logic.
armistace added 1 commit 2026-08-04 23:56:48 +10:00 
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Opencode commented 2026-08-04 23:57:32 +10:00
Author
Collaborator
Copy Link

Response to PR review — verified against actual code:

Thanks for the review. I want to note the three "critical" blockers are already handled in the code; the review was based on assumptions since it flagged it hadn't seen the patches. Verification:

1. Token storage — no plain-text persistence exists. ApiClient holds the session token in-memory only (private var bearerToken: String?); there is no SharedPreferences/DataStore/EncryptedSharedPreferences anywhere in the codebase. Nothing is written to disk, so there is nothing to encrypt. The app deliberately re-authenticates on each launch rather than persist a token — a stricter posture than encrypted storage.

2. No secrets are hardcoded. GOOGLE_AUTH_SETUP.md and the Kotlin sources contain only two Google OAuth Client IDs, which are public by design (they ship inside every native app and are not credentials). No client secret, API key, or private key exists anywhere in the repo.

3. IDOR on exports is mitigated server-side. Both export endpoints (and the job endpoint) in main.py check ownership: doc["user_email"] != user["email"] -> 403 Access denied, with admin bypass. downloadExport() calls those same endpoints with the Bearer token; a user cannot fetch another user's resume.

4. Cleartext — already disabled globally and per-domain in network_security_config.xml.
5. Tests — JobListModelsTest exists (4 passing); MyResumesViewModel/LoginViewModel already use viewModelScope.

Implemented as a result of this review: API base URL is now injected via buildConfigField (API_BASE) instead of hardcoded in ApiClient.kt, enabling per-environment builds.

Deferring: Hilt/DI (single-singleton dependency, adds a framework with no testability gain at this scale), SSL pinning/Play Integrity (future hardening, out of scope for this PR).

**Response to PR review — verified against actual code:** Thanks for the review. I want to note the three "critical" blockers are already handled in the code; the review was based on assumptions since it flagged it hadn't seen the patches. Verification: **1. Token storage — no plain-text persistence exists.** `ApiClient` holds the session token in-memory only (`private var bearerToken: String?`); there is **no SharedPreferences/DataStore/EncryptedSharedPreferences anywhere** in the codebase. Nothing is written to disk, so there is nothing to encrypt. The app deliberately re-authenticates on each launch rather than persist a token — a stricter posture than encrypted storage. **2. No secrets are hardcoded.** `GOOGLE_AUTH_SETUP.md` and the Kotlin sources contain only two Google OAuth **Client IDs**, which are public by design (they ship inside every native app and are not credentials). No client secret, API key, or private key exists anywhere in the repo. **3. IDOR on exports is mitigated server-side.** Both export endpoints (and the job endpoint) in `main.py` check ownership: `doc["user_email"] != user["email"]` -> `403 Access denied`, with admin bypass. `downloadExport()` calls those same endpoints with the Bearer token; a user cannot fetch another user's resume. **4. Cleartext** — already disabled globally and per-domain in `network_security_config.xml`. **5. Tests** — `JobListModelsTest` exists (4 passing); `MyResumesViewModel`/`LoginViewModel` already use `viewModelScope`. **Implemented as a result of this review:** API base URL is now injected via `buildConfigField` (`API_BASE`) instead of hardcoded in `ApiClient.kt`, enabling per-environment builds. Deferring: Hilt/DI (single-singleton dependency, adds a framework with no testability gain at this scale), SSL pinning/Play Integrity (future hardening, out of scope for this PR).
armistace merged commit e9ba43e09a into master 2026-08-04 23:58:13 +10:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
hermes Opencode armistace
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: armistace/resbuilder_app#2
No description provided.
Delete Branch "feature/my-resumes-modern"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?