Skip to content

Fix post-merge Devin Review findings on #72 - #108

Merged
sgardoll merged 1 commit into
milestone/stu-implement-updated-ui-uxfrom
devin/1790610789-post72-review-fixes
Sep 28, 2026
Merged

sgardoll merged 1 commit into
milestone/stu-implement-updated-ui-uxfrom
devin/1790610789-post72-review-fixes

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes the four Devin Review findings posted on the merged PR #72 commit (6ebe6e9).

Phantom dependencies from multiline Dart strings (dartPackageImports.js). ''' / """ literals were not consumed as tokens, so import 'package:x/x.dart'; inside a triple-quoted string scanned as a real directive and could add x to the project's pubspec. STRING_PATTERN now lists triple-quoted alternatives first (a '/" literal cannot span lines, so without them the string's interior lines scan as directives), and stripComments matches code.slice(i, i+3) before falling back to a single-char quote. Raw variants (r'''...''') are covered — the r is output bare and the quotes consume normally.

Comment braces closing flow maps early (pubspecSync.js). findFlowMappingEnd counted } inside YAML comments, so { + # close } later + sdk: flutter + } truncated at the comment's } and classified an SDK package unrepresentable. Both findFlowMappingEnd and parseFlowSourceDirective now honor the YAML comment rule (a # preceded by whitespace runs to end of line); the member segmenter excises comment text into a cleaned string before splitting, so a comment line can't masquerade as an unreadable member.

DEPLOYMENT.md policy drift. The verification section still described the dropped conservative policy ("any class importing a package: URI is reported unverified"). Rewritten to the merged semantics: direct importers of an unreproducible dep are reported with a reason; every other class compiles against the reduced scratch graph with one approximation notice per missing package; dart:-only classes are unaffected.

server.dart. Removed the second value.length > maxDependenciesPerRequest check in _normalizeSdkPackages — unreachable, and its hardcoded sdkPackages message was wrong for the sdkOverrides call site anyway.

Tests

  • 3 new regression tests: multiline ''' string quoting an import + a real import after it; raw """ variant; multiline flow map with } in a comment.
  • npm test 386/386, test:deploy-guard 9/9, test:proxies 30/30, npm run build clean.
  • No dist/ rebuild in this PR per repo convention.

Link to Devin session: https://app.devin.ai/sessions/616e9d5ba84144cfb7168de3eed8da26
Open in Devin Desktop: https://app.devin.ai/desktop/session/616e9d5ba84144cfb7168de3eed8da26?variant=devin
Requested by: @sgardoll


Devin Review

… flow-map scans

- SCAN_PATTERN and stripComments now consume '''...''' and """..."
  literals whole; a triple-quoted string quoting import text no longer
  invents a dependency (Devin Review on #72).
- findFlowMappingEnd and parseFlowSourceDirective skip YAML comments: a
  '}' inside a comment between flow-map members no longer closes the map
  early and the comment text no longer reads as an unreadable member.
- DEPLOYMENT.md now describes the merged scoped-skip + approximation
  policy instead of the dropped graph-level skip.
- server.dart: drop the duplicated maxDependenciesPerRequest check whose
  second branch was unreachable and hardcoded the wrong field name.

Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access that start with 'Devin'.

  • Disable automatic comment, CI, and merge conflict monitoring

@greptile-apps

greptile-apps Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

This PR does not match any of the 1 configured review trigger rule.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

E2E deploy-pipeline verification on this branch — drove the real app UI through the full deploy flow twice (npm run dev, ?debugBundle=multi fixture with 8 crafted classes) against a local stub for the FlutterFlow API + provision runner (no real credentials exist; all changed client code ran for real incl. live pub.dev resolution).

Run A success: 3 unverified warnings + approximation notice

Triple-quoted-string fix: a class carrying import 'package:ghost_*/ghost.dart' inside ''', r''', and """ strings — the confirm-modal deps list shows no phantom packages and the class stays verified:
Deps list — no ghost packages

YAML-comment fix: integration_test declared via a wrapped flow map containing # hosted: {url:...} comment text lands in sdkPackages, and meta via {version: ^1.14.0, # } ...} lands in dependencies as ^1.14.0 — comment braces and comment text no longer corrupt the parse.

Captured payloads + close-the-loop

Run A deployCustomClasses: sources=[agent_event, http_user, meta_user, itest_user, triple_quoted], sdkPackages=[flutter, flutter_localizations, integration_test], dependencies={http:^1.2.0, meta:^1.14.0, collection:^1.18.0}, dependencyOverrides={override_me:^1.3.0}, sdkOverrides=[]. Run B (path override): same sources verified approximately, dependencyOverrides={}, plus 2 disclosed "Verified approximately" notices (forked_pkg, override_me) — matching the scoped-skip + approximation policy the DEPLOYMENT.md rewrite describes. Pushed serialized_yaml preserves wrapped comment-laden flow maps verbatim and appends collection. Scratch package rebuilt from run-A manifest + sources: flutter pub get + dart analyze clean.

Not exercised: real api.flutterflow.io and the real Cloud Run compile runner (no credentials); DEPLOYMENT.md/server.dart have no local runtime surface. Verified by Devin session.

@sgardoll
sgardoll merged commit f1c819d into milestone/stu-implement-updated-ui-ux Sep 28, 2026
2 checks passed
sgardoll pushed a commit that referenced this pull request Sep 28, 2026
…u445-exact-html-fidelity

Re-sync STU-445 onto base f1c819d (merges #108 -> #104 -> #103).

Single conflict, in index.html, comment-only: the base adds a five-line
comment above <div class="pipeline-view"> documenting #104's
initPipelinePanelOrder focus-order contract. Resolved by taking the base
side's comment. The base's #104 CSS changes (dropping grid-template-areas
in favour of JS-driven region ordering) auto-merge and are preserved.

dist/ regenerated from the merged sources: app.js differs from both parents,
so the previous bundle was pre-merge. index-CD_p5Yhb.js -> index-C31N1DOp.js.
CSS is byte-identical (index-BQUorxWE.css), and a second build is a no-op.

npm test 388/388 green (the base contributes 3 tests over the prior 385).
No test changed, skipped, or weakened by this merge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant