75 lines
10 KiB
Markdown
75 lines
10 KiB
Markdown
# Final whole-branch review — undergraduate integration
|
||
|
||
Executor resolution (2026-10-15): all three Important findings and the record-type rendering failure (regraded Important) were reproduced RED then fixed GREEN; final Flutter suite249/249. Original independent report retained below. Live SSO remains unverified; all artifact checks and decisions are in tpschedule-0.7.0-validation.md.
|
||
|
||
Review range: `3f28e0a1b68a6a86d23c545ca143efb8e9431787..674df1ceb306fce13947c722e28b59188e8375ad`.
|
||
Date: 2026-10-15. This resumes the interrupted final review; it is not a second review or fix pass. Source, index, HEAD and branch state were not changed by this reviewer. Only this requested ignored review artifact was written inside the checkout. Temporary probes used the sandbox temp directory.
|
||
|
||
## Strengths
|
||
|
||
- Undergraduate imports have a separate connector, explicit manual timetable capture, and an all-types exam merge; the graduate workflow remains separately dispatched.
|
||
- School-qualified terms, stable persisted school IDs, period-12 source validation, per-page byte limits, narrow navigation origins, and inherited atomic persistence are coherent safeguards.
|
||
- Timetable cancellation subtraction and arrangement grouping preserve teachers, rooms and weeks. Undergraduate exam imports preserve the current timetable and date settings. Source labels and 11/12-row selection use saved data.
|
||
- The implementation includes focused parser, session, controller, widget and service regressions. The executor reports 241 Flutter tests, analysis and both Node probes green, native sample counts of 18 timetable arrangements and 4 examinations, and Android offline-flow checks. These are executor evidence, not independently rerun suite results in this review.
|
||
|
||
## Issues
|
||
|
||
### Critical
|
||
|
||
None identified in the reviewed range.
|
||
|
||
### Important
|
||
|
||
1. **Invalidate completed exam results when the browser starts a new document.**
|
||
- Location: `mobile/lib/features/school/undergraduate/session.dart:110–115`; related save guard at `mobile/lib/features/school/undergraduate/auth_page.dart:64–70`.
|
||
- `onPageStarted` clears only `pendingDraft`; in exam mode it leaves both `pendingExams` and `_generation` unchanged. `_complete` also stays true, so subsequent `_loaded` callbacks return before detecting an authentication change.
|
||
- Reproduction: finish an exam batch, then have a navigation to a different document (for example an authentication redirect) start before the deferred/queued save executes, or navigate after a failed save while the result remains pending. The captured generation and pending result identity still match. The page's `canApply` guard therefore accepts the obsolete exam result, and `session.saved(generation: oldGeneration)` also returns true. A fresh navigation or account transition cannot invalidate this completed result as required by the spec.
|
||
- Fix: separate document validity from intentional within-batch navigation, or explicitly invalidate completed/pending results on subsequent document starts; include document validity in the save guard. Keep normal type-to-type navigation working. Add a regression with a completed result, blocked controller queue, then new navigation before queue release.
|
||
- Evidence: direct control-flow inspection. A temporary external Flutter reproduction was prepared but did not execute assertions because the sandbox denied copying the compiled test artifact; no runtime pass is claimed for this issue.
|
||
|
||
2. **Preserve independently saved undergraduate exams when importing a new graduate timetable term.**
|
||
- Location: `mobile/lib/app/app_controller.dart:172–174`.
|
||
- The `clearExams` predicate checks the incoming timetable's school, but never checks the saved exam schedule's school. Whenever a graduate timetable changes term, it deletes the saved exam schedule even if that schedule is undergraduate data intentionally independent of the timetable.
|
||
- Reproduction: start with graduate timetable term 73; successfully import undergraduate exams for term 503; import graduate timetable term 72. `_differentTerm` is true and the incoming school is graduate, so `copyWith(clearExams: true)` persists removal of the undergraduate results and their successful query choices. The user requested a timetable update, not deletion of the independent exam query.
|
||
- Fix: preserve undergraduate exam schedules regardless of incoming timetable school; retain legacy graduate clearing only for graduate exam schedules to which that rule applies. Add this direction to the existing controller test, which currently tests only importing an undergraduate timetable after saving undergraduate exams.
|
||
- Evidence: direct evaluation of the persistence predicate and `copyWith` path. Same external Flutter test-run limitation as issue 1.
|
||
|
||
3. **Reject unconsumed activity/index syntax instead of silently importing a partial timetable.**
|
||
- Location: `mobile/lib/features/school/undergraduate/activity_data.dart:48–54` (also activity matching at lines 20–24).
|
||
- Completeness is measured by counting only already-recognized `index = ...;` and `tableN.activities[index][...] = activity;` statements. Unsupported additional activity writes are not included in either count, so they can silently disappear while the recognized subset passes validation. This violates the explicit unknown-index rejection and old-data-preservation requirement.
|
||
- Minimal reproduction: use a valid TaskActivity and the usual assignment at index `0*unitCount+0`, then append `table0.activities[1*unitCount+1][table0.activities[1*unitCount+1].length] = activity;`. The parser returns one activity with indices `{0}` and no error, dropping the Tuesday lesson. The timetable adapter has no subsequent full-script consumption validation; such a successful draft can replace a complete stored timetable and change reminders.
|
||
- Fix: tokenize/validate the complete relevant activity block, or detect every TaskActivity declaration and activities write and reject any unconsumed form. Supporting arbitrary JavaScript is unnecessary; rejecting unsupported syntax is sufficient. Cover a recognized statement followed by an unrecognized extra assignment, not only an unknown expression inside a recognized `index =` assignment.
|
||
- Evidence: an independent temporary Dart probe against the actual `parseActivityData` implementation ran successfully and printed `Parsed activities: 1; indices: {0}; direct second assignment silently omitted`. No private student data was used.
|
||
|
||
### Minor
|
||
|
||
4. **Validate the optional record-level exam type before accepting stored graduate records.**
|
||
- Location: `mobile/lib/domain/exam_schedule.dart:61` and `:116–120`; consumer `mobile/lib/features/exams/exams_page.dart:190` in the reviewed revision.
|
||
- `ExamRecord.fromJson` accepts any integer `examTypeId`. The schedule-level membership check runs only for undergraduate schedules, while the examination page calls `UndergraduateExamType.fromId` for every non-null record type, regardless of school.
|
||
- Reproduction: deserialize a graduate/default-school schedule with otherwise valid record fields and `examTypeId: 99`. Loading succeeds; opening its examination card throws `FormatException: 考试类型无效`. A damaged optional field therefore bypasses the intended invalid-save/backup recovery path and instead crashes the page rendering. This is lower likelihood because the current graduate adapter does not emit this field.
|
||
- Fix: reject non-null type IDs outside 1–3 at the model boundary, and/or require the field to be absent for graduate data. Keep old records with no field compatible.
|
||
- Evidence: independent Dart probe loaded type 99 successfully and reproduced the enum conversion exception used by the page.
|
||
|
||
## Verification and limitations
|
||
|
||
- Read the production diff, spec, plan, progress ledger, relevant existing tests, and validation record. Inspected the exact code paths above.
|
||
- Two pure-Dart reproduction cases ran against the implementation: unsupported activity assignment omission and invalid record-type acceptance/render conversion.
|
||
- Three additional external temporary Flutter probes were attempted for the controller, completed-session invalidation and full timetable adapter. Flutter failed before assertions with an output.dill/listener.dart.dill copy permission error in the sandbox temp directory. The process was stopped. This is a test-environment limitation, not a product test failure, and those probes are not counted as executed regressions.
|
||
- The semicolon-omission parser candidate was considered and rejected as a finding: the tested form threw a FormatException rather than silently importing. The confirmed parser finding above uses an unrecognized extra assignment instead.
|
||
|
||
## Recommendations
|
||
|
||
Fix issues 1–3 before shipping, adding narrow regressions for their triggers. Issue 4 is a small model-boundary fix consistent with the corrupt-data requirement. Run the relevant session/controller/parser/model tests and the usual final checks after fixes. Rebuild artifacts if shipped source changes. No second independent review pass is requested by this report.
|
||
|
||
## Declined to judge
|
||
|
||
- Actual undergraduate SSO, CAPTCHA and live school callback behavior: no authenticated school session was available; offline fixtures and fake WebView callbacks cannot establish successful real authentication. This remains a documented release validation boundary, not a claimed pass.
|
||
- Query-label/card spacing and the glass offset/shader changes made after `674df1c`: explicitly separate current user requests, outside this fixed review range, being handled by the executor.
|
||
- Frozen registration materials, private signing keys and current APK signature/ZIP contents: this is the requested source-branch safety review; artifact verification is recorded by the executor. No private key or original student HTML was opened or printed by this reviewer.
|
||
|
||
## Assessment
|
||
|
||
**Ready to merge: With fixes.**
|
||
|
||
The core separation and persistence design follows the plan, but document invalidation, cross-school exam retention and fail-closed activity parsing have reproducible gaps that can save obsolete data or remove valid data. Resolve the three Important findings before delivering the updated build; the Minor finding is also straightforward to address.
|