docs(uipath-review): Check App State only to branch, Finally runs only when caught in the same file - #3786

Claude finished @danmunteanu1's task in 2m 47s —— View job

PR Review: docs(uipath-review) — Check App State, Finally scope, hard-coded delays

• Read review criteria and project conventions
• Analyze full diff
• Read changed files in full for context
• Check for cross-skill references, broken links, secrets
• Post review findings

Summary

Upgrades "Check App State / Element Exists" from Info to Warning (inverted: a check is valid only when absence leads to different work), adds a new common-issues section for the pattern, clarifies that Finally runs only when a Catch in the same file handles the exception, and corrects the hard-coded delay fix to point at the next activity's Timeout instead of adding a check.

Change-by-Change Review

1. skills/uipath-review/references/rpa/rpa-review-checklist.md (line 163)

Severity: Low

Check column updated from short Info text to a Warning-level description with a link to the new common-issues section. Content is accurate and the link resolves correctly. The cell is denser than its neighbours in the table, but stays scannable.

2. skills/uipath-review/references/rpa/rpa-advanced-checklist.md (lines 78, 137)

Severity: Low

Two table cells updated — Selector Resilience and Finally Block Usage. Both upgrade severity, expand the Check text, and add links to rpa-common-issues.md. Links resolve to the correct section headings. Same minor density observation as above.

The Finally row (line 137) packs a full behavioral rule into a table cell ("in Finally where a Catch in the same file handles the exception, at the end of Try and in each Catch before Rethrow where the exception leaves the file"). The Check column now carries as much detail as the common-issues Fix paragraph. Consider shortening the cell to the rule name + link and leaving the full rule in common-issues — but this is Low/polish since the table is still parseable.

3. skills/uipath-review/references/rpa/rpa-common-issues.md (lines 79, 81-89, 287-293)

Severity: OK

Three changes:

• Hardcoded Delays fix (line 79): Updated from "use Check App State" to "rely on the next UI activity's Timeout." Adds the key insight that a Check App State before the same element is the same defect. Clean inversion, well-worded.

• New section "Unnecessary Check App State / Element Exists" (lines 81-89): Follows the existing Symptom/Impact/Detection/Fix pattern. Detection advice (NCheckState / ElementExists grep, target comparison) is concrete and actionable. The Fix paragraph (line 89) is dense — it covers delete-the-check, keep-for-branching, optional-popup pattern, OutUiElement pass-through, and VerifyOptions outcome checks in one paragraph. Each clause carries information, but an agent processing this in a review loop may benefit from a numbered list or sub-bullets. Low priority since the information is correct and complete.

• Missing Finally Blocks (lines 287-293): Symptom expanded to cover "cleanup only in Finally when exceptions leave the file." Detection and Fix rewritten around the scope rule. The Fix paragraph (line 293) is the most domain-critical text in the PR — it asserts that Finally is skipped when an exception leaves the file, even under ContinueOnError or Isolated. Flagged for manual verification below.

What's Missing

Nothing identified. The three files that carry Check App State / Finally guidance are all updated consistently. No new skill folder, no CODEOWNERS change needed.

Area Ratings

Issues for Manual Review

1. Finally scope claim (rpa-common-issues.md:293): The PR asserts "a Finally runs only when a Catch in the same workflow file handles the exception … an exception that leaves the file (rethrown, thrown from a Catch, or never caught) skips its Finally, even when the invoking workflow catches it, the Invoke Workflow File sets ContinueOnError, or it runs Isolated, because Invoke Workflow File runs each file as its own workflow instance." This is a strong behavioral claim about UiPath's runtime. If correct, it's a high-value insight that prevents a class of resource-leak bugs. A domain expert should confirm against the UiPath runtime or the UIA package docs.

2. Hard-coded delay fix inversion (rpa-common-issues.md:79): The old guidance recommended Check App State as the fix for hard-coded delays. The new guidance says that's "the same defect in another form" and points to the activity's own Timeout instead. This is a meaningful change in recommended practice — worth a domain-expert sanity check.

3. VerifyOptions carve-out (rpa-common-issues.md:89): The Fix text carves out specific activities (Click, Hover, Keyboard Shortcuts, Type Into) for VerifyOptions and names others (Select Item, Check/Uncheck, Go To URL) where a post-action check is the assertion. Verify this partition matches the UIA package's VerifyOptions availability.

Conclusion

Clean, well-scoped PR that upgrades review criteria based on real genome findings. No structural, consistency, or hygiene issues. The three manual-review items are domain-correctness checks that an automated reviewer can't verify — the writing quality and skill-convention compliance are solid. Approve, pending domain confirmation of the Finally scope rule and the VerifyOptions activity partition.