Use AppBuilders Version in key for project_data cache#917
Conversation
WalkthroughThe workflow adds a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
68f26e4 to
859e352
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/main.yml (1)
10-11: Clarify job dependencies and gating behavior.The
check-appbuilders-versionjob does not explicitly depend on other jobs, so it runs in parallel withsetup,lint, andtest. Confirm whether:
- This version check should gate all downstream jobs (making
setup,lint, andtestdepend on it), or- It's intended as an informational check that runs independently.
If it should gate the workflow, add a
needsconstraint:- check-appbuilders-version: - runs-on: ubuntu-latest + check-appbuilders-version: + runs-on: ubuntu-latest + # Consider adding explicit gate if this should block downstream jobs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/main.yml(1 hunks)package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-16T18:49:02.188Z
Learnt from: judah-sotomayor
Repo: sillsdev/appbuilder-pwa PR: 840
File: src/lib/components/NoteDialog.svelte:94-95
Timestamp: 2025-06-16T18:49:02.188Z
Learning: The appbuilder-pwa project uses Svelte 5, as evidenced by "svelte": "^5" in package.json and the use of Svelte 5 runes syntax like $props() and $derived(). In Svelte 5, onclick is the preferred event handler syntax over on:click.
Applied to files:
package.json
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (1)
package.json (1)
3-3: Verify that version jump from 0.0.1 to 13.3 doesn't break downstream tooling.The version was updated to match app-builders' GHCR tag, but this represents a significant semantic version jump (0.0.1 → 13.3). Confirm that deployment scripts, release pipelines, or any downstream consumers that expect gradual version progression can handle this change.
Try container id? Try tags API? Full check? Move check to main.yml Put latest version of PWA in package.json Apparently I needed to format the yml.... Include appbuilder version in project cache key Fix cache key Revert "Put latest version of PWA in package.json"
ae43916 to
85c3dc1
Compare
chrisvire
left a comment
There was a problem hiding this comment.
Any caching of project generation should be based on the appbuilder_verison
Tests were broken when app-builders added some new features. This does not fix those tests. This allows broken tests to be caught earlier by making sure that the latest version of app-builders is used to extract the project data for testing. Some of this was delayed by the fact that we were caching the extracted data to save time during the testing process. Using the version number as part of the cache key bypasses this issue.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.