GitHub User Experience Enahncement#59
Conversation
|
The tests are still failing because of a permissions issue regarding the pipeline adding comments to the github PR. Maybe this would work if it were coming from a branch on this repo and not an outside fork? |
There was a problem hiding this comment.
Pull request overview
This PR introduces a real-time event pipeline (Redis pub/sub → Django SSE → browser EventSource) to surface GitHub pull/build status in the UI, and refactors GitHub pulls to support incremental “delta” sync (with a full re-import fallback/option) plus auto-build behavior parity with webhook-triggered pulls.
Changes:
- Added SSE infrastructure: Redis event publisher, authenticated SSE endpoint, nginx streaming support, and browser-side
sse.jshandlers wired into the project page. - Implemented GitHub delta pull flow and a “wipe on pull” option (full re-import via
wipe_existing=True), plus additional tests around SSE, git sync, and archive import semantics. - Updated UI/UX: sidebar pending indicators, improved pull confirmation dialog, GitHub pane settings and status display, and “no full page reload” sidebar refresh.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents GITHUB_HOOK_URL and ngrok workflow for local webhook callbacks. |
| nginx/nginx.conf | Adds a streaming-friendly nginx location for the SSE endpoint. |
| docker-compose.yml | Adds optional .env.local loading for web and celery. |
| cloudpebble/utils/events.py | Introduces Redis pub/sub event publishing helper. |
| cloudpebble/ide/utils/cloudpebble_test.py | Removes explicit test environment setup call. |
| cloudpebble/ide/urls.py | Registers the new per-project SSE endpoint route. |
| cloudpebble/ide/tests/test_sse.py | Adds unit tests for SSE formatting, auth, and event publishing from tasks. |
| cloudpebble/ide/tests/test_import_archive.py | Adds tests for wipe_existing behavior and rollback safety. |
| cloudpebble/ide/tests/test_git.py | Adds extensive tests around git pull routing and delta sync helpers. |
| cloudpebble/ide/templates/ide/project/github.html | Adds “wipe on auto-pull” UI and GitHub last-sync display area. |
| cloudpebble/ide/templates/ide/project.html | Updates pull modal copy/controls and includes sse.js. |
| cloudpebble/ide/tasks/git.py | Implements delta sync, force/full pull routing, and publishes pull/build SSE events. |
| cloudpebble/ide/tasks/build.py | Publishes build completion SSE events. |
| cloudpebble/ide/tasks/archive.py | Adds wipe_existing option to delete existing files/resources during import. |
| cloudpebble/ide/static/ide/js/sse.js | Adds client-side SSE connection and event dispatch to existing handlers. |
| cloudpebble/ide/static/ide/js/sidebar.js | Adds pending-pill indicator helpers and a sidebar “refresh” method. |
| cloudpebble/ide/static/ide/js/github.js | Updates GitHub pane UI, adds pull force option, and SSE-driven pull callbacks. |
| cloudpebble/ide/static/ide/js/compile.js | Adds compile pane hooks for build start/complete events. |
| cloudpebble/ide/static/ide/js/cloudpebble.js | Initializes the SSE client during project init. |
| cloudpebble/ide/static/ide/js/tests/sse-events.test.js | Adds JS tests verifying SSE handler behavior. |
| cloudpebble/ide/static/ide/css/ide.css | Styles the sidebar pending pill. |
| cloudpebble/ide/models/project.py | Adds github_hook_force project setting. |
| cloudpebble/ide/migrations/0013_github_hook_force.py | Migrates the new github_hook_force field. |
| cloudpebble/ide/api/sse.py | Adds authenticated SSE streaming endpoint implementation. |
| cloudpebble/ide/api/project.py | Exposes hook_force in project info payload. |
| cloudpebble/ide/api/git.py | Adds force/full-pull parameter plumbing and persists hook-force setting. |
| cloudpebble/cloudpebble/settings.py | Adds GITHUB_HOOK_URL override for webhook callback base URL. |
| cloudpebble/.gitignore | Ignores node_modules/ in the cloudpebble subdir. |
| .gitignore | Ignores .env.local at repo root. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resource_root = project.resources_path + '/' | ||
| manifest_resources = manifest.get('resources', {}).get('media', []) | ||
| project_type = manifest.get('projectType', 'native') | ||
|
|
||
| for resource in manifest_resources: | ||
| path = resource_root + resource['file'] | ||
| if project_type == 'pebblejs' and resource['name'] in PEBBLEJS_BUILTIN_RESOURCES: | ||
| continue | ||
| if path not in paths_notags: | ||
| raise Exception("Resource %s not found in repo." % path) |
There was a problem hiding this comment.
this should be resolved now
| manifest_kind = 'package.json' if 'pebble' in manifest else 'appinfo.json' | ||
| resource_root = project.resources_path + '/' | ||
|
|
||
| with transaction.atomic(): | ||
| project_options, media_map, dependencies = load_manifest_dict(manifest, manifest_kind) | ||
|
|
||
| for k, v in project_options.items(): | ||
| setattr(project, k, v) | ||
| project.full_clean() | ||
| project.set_dependencies(dependencies) | ||
|
|
||
| tag_map = {v: k for k, v in ResourceVariant.VARIANT_STRINGS.items() if v} | ||
|
|
||
| existing_sources = {s.project_path: s for s in project.source_files.all()} | ||
| existing_resources = {r.file_name: r for r in project.resources.all()} | ||
|
|
||
| for change in changed_files: | ||
| filename = change.filename | ||
| status = change.status | ||
|
|
||
| if status in ('added', 'modified', 'renamed'): | ||
| if status == 'renamed' and change.previous_filename: | ||
| _remove_file_by_path(project, change.previous_filename, existing_sources, existing_resources) | ||
|
|
||
| if filename.startswith(resource_root): | ||
| _upsert_resource_variant(project, repo, change, existing_resources, tag_map) | ||
| else: | ||
| try: | ||
| base_filename, target = SourceFile.get_details_for_path(project.project_type, filename) | ||
| _upsert_source_file(project, repo, change, base_filename, target, existing_sources) | ||
| except ValueError: | ||
| logger.debug("Skipping unrecognized file in delta: %s", filename) | ||
| continue |
There was a problem hiding this comment.
this should be resolved now
| tag_map = {v: k for k, v in ResourceVariant.VARIANT_STRINGS.items() if v} | ||
|
|
||
| existing_sources = {s.project_path: s for s in project.source_files.all()} | ||
| existing_resources = {r.file_name: r for r in project.resources.all()} |
There was a problem hiding this comment.
this should be resolved now
|
|
||
| project.save() | ||
|
|
||
|
|
There was a problem hiding this comment.
this should be resolved now
| CloudPebble.Editor.GetUnsavedFiles = function() { return 0; }; | ||
| $('#sidebar-sources').empty(); | ||
| $('#sidebar-resources').empty(); | ||
| create_initial_sections(CloudPebble.ProjectInfo.type); | ||
| Ajax.Get('/ide/project/' + PROJECT_ID + '/info').then(function(data) { | ||
| CloudPebble.ProjectInfo = data; | ||
| var is_alloy = data.type === 'alloy'; | ||
| $.each(data.source_files, function(index, value) { | ||
| if (is_alloy && value.target === 'embeddedjs' && value.is_binary) { | ||
| if (CloudPebble.Resources && _.isFunction(CloudPebble.Resources.AddAlloyAsset)) { | ||
| CloudPebble.Resources.AddAlloyAsset(value); | ||
| } | ||
| return; | ||
| } | ||
| CloudPebble.Editor.Add(value); | ||
| }); | ||
| $.each(data.resources, function(index, value) { | ||
| CloudPebble.Resources.Add(value); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
this should be resolved now
| @@ -43,6 +44,7 @@ def set_project_repo(request, project_id): | |||
| branch = request.POST['branch'] | |||
| auto_pull = bool(int(request.POST['auto_pull'])) | |||
| auto_build = bool(int(request.POST['auto_build'])) | |||
| hook_force = bool(int(request.POST.get('hook_force', '0'))) | |||
|
|
|||
There was a problem hiding this comment.
this should be fixed now
| def publish_event(project_id, event_type, **kwargs): | ||
| data = {'type': event_type} | ||
| data.update(kwargs) | ||
| channel = 'project_events:{}'.format(project_id) | ||
| redis_client.publish(channel, json.dumps(data)) | ||
| logger.debug("Published %s to %s", event_type, channel) No newline at end of file |
There was a problem hiding this comment.
this should be fixed now
|
@jplexer i think i have remedied all of the comments made by autipilot. Can you have it re-review to see if it thinks all of its concerns have been satisfied? |
|
@jplexer can you try deploying to a dev instance and double checking it works pls |
|
@jplexer bumping this up - please test out this week |
|
After your bug fix I had to make some changes and I haven't personally had
any time to really test them personally.
Sincerely,
Justin Sunseri
…On Tue, Jun 9, 2026, 5:11 AM Eric Migicovsky ***@***.***> wrote:
*ericmigi* left a comment (coredevices/cloudpebble#59)
<#59 (comment)>
@jplexer <https://github.com/jplexer> bumping this up - please test out
this week
—
Reply to this email directly, view it on GitHub
<#59?email_source=notifications&email_token=AAGEPF3OCDGXJJ2YZ7Q5FLD464MWVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRVGMYDKMRSGU22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4653052255>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGEPF3B453PMFATGTHDXPL464MWVAVCNFSM6AAAAACZLR5UVWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DMNJTGA2TEMRVGU>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AAGEPF2KWD2SAMZSVRIWFT3464MWVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRVGMYDKMRSGU22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
and Android
<https://github.com/notifications/mobile/android/AAGEPFYCB6ZWVABKBNPM7KL464MWVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRVGMYDKMRSGU22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
Download it today!
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Remove setup_test_environment() call that conflicted with Django's test runner (was the reason these tests were .skip-ed). Rename test_import_archive.skip to .py. Add TestWipeExisting test class covering the new wipe_existing parameter: replaces source files and resources, preserves files on default (False), and rolls back on failure.
…elpers Start zip download in parallel with tree/manifest validation using ThreadPoolExecutor. Move the SHA check before any network work so unchanged repos return instantly without downloading anything. Extract validate_resources_against_tree() and parse_manifest_from_tree() as pure functions from github_pull. Extract PEBBLEJS_BUILTIN_RESOURCES as a frozenset constant. Add 24 unit tests covering get_root_path, resource validation, Pebble.js builtins, and manifest parsing including edge cases for variant suffixes, missing resources, subdirectories, package.json, and invalid JSON.
When github_pull detects a change between the stored commit and the remote branch, it now uses GitHub's Compare API to fetch only the files that changed, then applies minimal DB updates (add/modify/remove SourceFile and ResourceFile/ResourceVariant records) within an atomic transaction. A force=True parameter (exposed via checkbox in the pull modal) falls back to the existing full-zip wipe-and-replace path. This also serves as the path for initial pulls where github_last_commit is None. Extracted testable helpers: validate_resources_against_tree, parse_manifest_from_tree, _apply_delta_changes, _upsert_source_file, _upsert_resource_variant, _remove_file_by_path, _sync_resource_files_from_manifest, _fetch_file_content, _infer_resource_kind_from_path, _strip_resource_dir_prefix. Added 47 unit tests covering the new helpers, including FetchFileContentTest and RemoveFileByPathTest with mocked GitHub API. Fixed Django template syntax in github-pull-prompt modal.
Refactor SSE event handlers into a separate module for testability. Export handlers from the SSE module and add unit tests for both the JavaScript event handlers and the Python SSE event publishing logic.
- Emit proper SSE event types instead of embedding in JSON data - Replace spinning icon with animated "Pulling"/"Building" pending pill - Close pull prompt immediately on confirm; remove page reload hack - Move force-pull warning behind checkbox toggle - Auto-trigger build after webhook-initiated pull - Add GITHUB_HOOK_URL setting for ngrok in local development - Fix GitTree attribute access (x.path instead of x)
Moves the full page reload logic from OnPullComplete into a new CloudPebble.Sidebar.Refresh function. This updates project info and repopulates source/resource lists without losing editor state. Also removes `if (pane)` guards in github.js and adds docstrings to git test methods.
The validation function now supports `package.json` manifests where resources are nested under the `pebble` key. It also correctly handles projects cloned into a subdirectory by prepending the `root` path to the resource lookup. Fix a trailing parenthesis in README.
Use the full manifest path (including the resource directory prefix) when keying the existing resources dictionary, so that delta changes are correctly matched against the modified file paths.
When applying delta changes, pass media_map to _upsert_resource_variant so the correct resource kind is used from the manifest instead of inferring from the path. When syncing from manifest, update kind and menuIcon on existing resources if they differ.
Add pyrightconfig.json
Catch RedisError, ConnectionError and generic connection failures to prevent crashes when Redis is unavailable.
Use try/finally in the done callback to guarantee GetUnsavedFiles is restored even if an error is thrown. Also restore it on ajax failure.
Move was_clean to code_mirror instance so the state survives editor detach. Only reload the active buffer when it has no unsaved edits, preventing user work from being clobbered after a GitHub pull.
Wrap github_last_commit and github_last_sync saves in transaction.atomic() across full, delta, and no-new-commit pull paths so a partial failure never leaves the project with new files but the old SHA.
- Re-add wipe_existing param to do_import_archive (dropped with P0 commit since coredevices subsumed it, but tests still use it) - Update GithubPullFullAtomicityTest to mock find_project_root_and_manifest and load_manifest_dict instead of the removed parse_manifest_from_tree
jmsunseri
force-pushed
the
github-performance
branch
from
d62d08f to
0cd9fbe
Compare
|
Sorry I only got to look at this now. Tested locally with a real repo and hit a blocker: delta pull never applies file changes. Also Worth guarding too: compare() is three-dot, so a force-push backwards gives ahead_by=0 with empty files and it stamps the SHA without applying anything. And comparison.files caps at 300. Falling back to full pull on status != 'ahead' or >=300 files covers both. The delta tests mock repo entirely, which is why this slipped through. Happy to re-test once fixed. Rest of the PR (SSE, pills, no-reload refresh) worked nicely for me. |
Use get_git_blob instead of get_contents to fetch file content since the SHA is a blob ref, not a commit ref. Also validate comparison status and file count before delta syncing.
excellent catch! i kept modifying the a README.md over and over again for testing but that file doesn't appear in the source view of the project so i never actually noticed that my changes weren't being included. This should be resolved now. In addition to that I had to change the docker compose to get testing this to work again locally. Let me know if those changes cause you any issues running locally. If you don't already you will need to make sure you have an .env.local with GITHUB_SYNC_CLIENT_ID and GITHUB_SYNC_CLIENT_SECRET defined. |
Summary
This MR adds real-time GitHub pull and build notifications via Server-Sent Events (SSE), introduces incremental delta sync for GitHub pulls, and fixes several bugs discovered along the way.
SSE Event System
/ide/project/<id>/events): Streams real-time events to the browser using Redis pub/sub. Events are published by Celery tasks (pull_start,pull_complete,pull_failed,build_start,build_complete) and delivered as properly formatted SSE named events (event: pull_start\ndata: {...}\n\n).sse.js: Connects to the SSE endpoint and dispatches events to handler functions exposed onCloudPebble.Events. Handlers update the sidebar with pending pill indicators (e.g., "Pulling...", "Building..." with animated dots) and trigger appropriate UI updates.utils/events.py:publish_event(project_id, event_type, **kwargs)utility for publishing events to Redis.Sidebar Pending Indicators
ShowPending/HidePending). These cycle through "Pulling" → "Pulling." → "Pulling.." → "Pulling..." and disappear cleanly on completion. The pills use the site's orange accent color (#ff4700).GitHub Pull Improvements
do_github_pullnow publishes SSE events and triggers an automatic build whengithub_hook_buildis enabled, matching the behavior of webhook-triggered pulls. Previously, manual pulls never triggered a build.get_root_path(x)was passingGitTreeElementobjects instead ofx.pathstrings in two locations in_github_pull_full, causing delta comparisons to fail.OnPullComplete/OnPullFailed/OnPullStarthandlers referenced a localpanevariable that was undefined outsideshow_github_pane(). Fixed to use direct jQuery selectors.window.location.reload(true),OnPullCompletecallsCloudPebble.Sidebar.Refresh()which silently re-fetches the file list from the server and rebuilds the sidebar without disrupting the user's current view. No navigation to the GitHub pane occurs.Build Notifications
build_startevents show a "Building..." pill in the sidebar.build_completeclears it.build_start— the pill is sufficient feedback.Configuration & Infrastructure
GITHUB_HOOK_URLenv var: Separate setting for GitHub webhook callback URL (via ngrok in dev), independent ofPUBLIC_URLwhich must remainlocalhostfor Firebase Auth.proxy_buffering off,proxy_cache off,proxy_read_timeout 86400, andchunked_transfer_encoding offto ensure proper streaming.docker-compose.yml: AddedGITHUB_HOOK_URLto environment config.GITHUB_HOOK_URLinstructions.Tests
test_sse.py, 20 tests): CoversSSEEventStreamformatting,publish_event, the SSE view auth/project checks,hooked_commitevent publishing,do_github_pullevent publishing and auto-build behavior, andrun_compileevent publishing.test_git.py, 67 tests): CoversGetRootPath,ValidateResourcesAgainstTree,ParseManifestFromTree,PebblejsBuiltins,InferResourceKind,StripResourceDirPrefix,FetchFileContent,RemoveFileByPath,GithubPullRoutingTest(force→full, delta, fallback, no-change, etc.),GithubPullDeltaTest,ApplyDeltaChangesTest,UpsertSourceFileTest,UpsertResourceVariantTest.sse-events.test.js, 8 tests): Loads realsse.jsviareadFileSync+new Function(), verifies handlers callShowPending/HidePendingandOnPullStart/OnPullComplete/etc. with correct arguments.Migration
0013_github_hook_force.py: Addsgithub_hook_forceboolean field to the Project model.New GitHub pull settings
The text here changes based on the checked or unchecked status
Warning if you check the box when manual pull
Default pull now only fetches the delta
Off camera i'm pushing a commit to github and you can see the label on the left that github is pulling then building. Notice testing.c stays because we did not have the force check box checked.
ScreencastAutomatic.webm.mp4
Now when you do a manual pull and the setting is set to automatically build it will actually automatically build. Notice the testing.c file goes away because we had the checkbox checked
ScreencastManual.webm.mp4