Repository navigation
Warn before saving an agent rule that RuleBot can't carry out - #104807
yuwenmemon wants to merge 4 commits into
Conversation
🦜 Polyglot Parrot! 🦜Squawk! Looks like you added some shiny new English strings. Allow me to parrot them back to you in other tongues: View the translation diffdiff --git a/src/languages/de.ts b/src/languages/de.ts
index c68d5bff..0b2355d4 100644
--- a/src/languages/de.ts
+++ b/src/languages/de.ts
@@ -8823,7 +8823,7 @@ Fügen Sie weitere Ausgabelimits hinzu, um den Cashflow Ihres Unternehmens zu sc
createRule: 'Regel erstellen',
createAnyway: 'Trotzdem erstellen',
saveAnyway: 'Trotzdem speichern',
- checkingRule: 'Ihre Regel wird geprüft...',
+ checkingRule: 'Ihre Regel wird überprüft ...',
fallbackBanner:
'Dies ist keine Standardregel, daher haben wir sie als Agentenregel erstellt. Agentenregeln greifen, nachdem sich eine Ausgabe oder ein Bericht geändert hat, zum Beispiel indem sie die Ausgabe zurückhalten oder den Bericht ablehnen.',
fallbackBannerWithExplanation: (explanation: string) => `Dies ist keine Standardregel, daher haben wir sie als Agentenregel formuliert. ${explanation}`,
diff --git a/src/languages/fr.ts b/src/languages/fr.ts
index ca9efc6e..a107fdf5 100644
--- a/src/languages/fr.ts
+++ b/src/languages/fr.ts
@@ -8846,7 +8846,7 @@ Ajoutez davantage de règles de dépenses pour protéger la trésorerie de l’e
createRule: 'Créer une règle',
createAnyway: 'Créer quand même',
saveAnyway: 'Enregistrer quand même',
- checkingRule: 'Vérification de votre règle...',
+ checkingRule: 'Vérification de votre règle…',
fallbackBanner:
'Ce n’est pas une règle standard, nous l’avons donc rédigée comme une règle d’agent. Les règles d’agent agissent après la modification d’une dépense ou d’une note de frais, par exemple en bloquant la dépense ou en rejetant la note de frais.',
fallbackBannerWithExplanation: (explanation: string) => `Ce n’est pas une règle standard, nous l’avons donc rédigée comme une règle d’agent. ${explanation}`,
diff --git a/src/languages/ja.ts b/src/languages/ja.ts
index 87b560c1..56b1861f 100644
--- a/src/languages/ja.ts
+++ b/src/languages/ja.ts
@@ -8661,9 +8661,9 @@ ${reportName}`,
describeRuleForConcierge: 'ルールの内容を入力すると、Concierge が自動作成します',
gotIt: '了解しました',
createRule: 'ルールを作成',
- createAnyway: 'このまま作成',
+ createAnyway: 'それでも作成',
saveAnyway: 'このまま保存',
- checkingRule: 'ルールを確認しています...',
+ checkingRule: 'ルールを確認しています…',
fallbackBanner:
'これは標準ルールではないため、エージェントルールとして作成しました。エージェントルールは、経費やレポートに変更があった後に動作し、例えば経費を保留にしたり、レポートを却下したりします。',
fallbackBannerWithExplanation: (explanation: string) => `これは標準ルールではないため、エージェントルールとして作成しました。${explanation}`,
diff --git a/src/languages/pl.ts b/src/languages/pl.ts
index 1e9357e7..a5d6be34 100644
--- a/src/languages/pl.ts
+++ b/src/languages/pl.ts
@@ -8778,7 +8778,7 @@ Dodaj więcej zasad wydatków, żeby chronić płynność finansową firmy.`,
createRule: 'Utwórz regułę',
createAnyway: 'Utwórz mimo to',
saveAnyway: 'Zapisz mimo to',
- checkingRule: 'Sprawdzanie reguły...',
+ checkingRule: 'Sprawdzanie twojej reguły...',
fallbackBanner:
'To nie jest standardowa reguła, więc przygotowaliśmy ją jako regułę agenta. Reguły agenta działają po zmianie wydatku lub raportu, na przykład wstrzymując wydatek albo odrzucając raport.',
fallbackBannerWithExplanation: (explanation: string) => `To nie jest standardowa reguła, więc przygotowaliśmy ją jako regułę agenta. ${explanation}`,
diff --git a/src/languages/zh-hans.ts b/src/languages/zh-hans.ts
index 50b347c7..2ad2a549 100644
--- a/src/languages/zh-hans.ts
+++ b/src/languages/zh-hans.ts
@@ -8452,7 +8452,7 @@ ${reportName}`,
createRule: '创建规则',
createAnyway: '仍然创建',
saveAnyway: '仍然保存',
- checkingRule: '正在检查你的规则...',
+ checkingRule: '正在检查您的规则…',
fallbackBanner: '这不是一条标准规则，所以我们将其制定为代理规则。代理规则会在报销或报表发生变更后生效，例如暂扣该报销或驳回该报表。',
fallbackBannerWithExplanation: (explanation: string) => `这不是一条标准规则，因此我们将其起草为代理规则。${explanation}`,
},
Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@marufsharifi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Okay @JS00001 @rushatgabhane this is ready to go |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| savePrompt: saveRule, | ||
| }); | ||
|
|
||
| const trySuggestedPrompt = (suggestedPrompt: string) => { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The agent-rule-check wiring is duplicated between EditAgentRulePage.tsx and AddAgentRuleWriteTab.tsx: both declare the same isChecking subscription, call useAgentRulePromptCheck, define an identical trySuggestedPrompt helper (formRef.current?.resetForm(...) + checkSuggestedPrompt(...)), and render the identical ImpossibleRuleNotice + disclaimer Text block (with the same checkingRule/disclaimer ternary). When this glue changes, both files have to be kept in sync by hand.
Consider extracting the shared footer and helper into one small shared component so both pages render it identically:
// Shared by AddAgentRuleWriteTab and EditAgentRulePage
function AgentRulePromptFooter({refusal, isChecking, onTrySuggestion}: AgentRulePromptFooterProps) {
const {translate} = useLocalize();
const styles = useThemeStyles();
return (
<>
{!!refusal && <ImpossibleRuleNotice refusal={refusal} onTrySuggestion={onTrySuggestion} />}
<Text style={[styles.textMicroSupporting, styles.textAlignCenter, styles.mt2]}>
{translate(isChecking ? 'workspace.rules.agentRules.checkingRule' : 'workspace.rules.agentRules.disclaimer')}
</Text>
</>
);
}Reviewed at: eada752 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
(Yuwen's Agent) Fixed in ad0ad01. The hook now owns the isChecking subscription and resets the form for Try this instead, so both pages only call it and render ImpossibleRuleNotice.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eada7520e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
|
|
||
| /** Hides the refusal when the admin changes the text, so that the next save checks the new text */ | ||
| const dismissRefusal = () => setCheckGenerationID(undefined); |
There was a problem hiding this comment.
If the admin edits the still-enabled input while a check is running, this callback only hides the refusal; it leaves pendingCheckRef and its timeout active. A non-impossible response or the timeout then saves pendingCheck.prompt—the text submitted before the edit—and navigates away, silently discarding the newer input in both the add and edit flows. Cancel the pending check when the value changes, or prevent editing until it finishes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
(Yuwen's Agent) Fixed in ad0ad01. Editing the text now stops the running check and its timeout, and ends the loading state, so the answer for the old text can't save it. The admin taps Create rule or Save again to check the new text. Added a test for it.
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
This change makes sense to me from a product perspective.
Explanation of Change
Admins can type an agent rule straight into the agent rule page, and the AI rules builder never sees it. A rule that RuleBot can't carry out still saves, adds RuleBot as an admin and never runs. This checks a typed or edited rule with
GenerateRulewhen the admin taps Create rule or Save. Animpossibleanswer shows the describe box's refusal, and the button becomes Create anyway or Save anyway, because this only warns. Any other answer, a failed check, or no answer within 20 seconds saves as before. A suggestion or a describe flow draft that the admin leaves unchanged skips the check, because Concierge wrote it.The check runs for every admin, because App#104861 removed the
describeRulebeta. Web-Expensify#56886, which letsGenerateRuleanswerimpossible, is in production.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/683438
PROPOSAL: N/A
Tests
The backend answers
impossibleonly after Web-Expensify#56886, so the snippet below swaps the backend's answer foranswerin the browser and keeps itsgenerationID.answer = {state: 'impossible', reason: 'noCapability', explanation: "Agents can't send text messages.", suggestedPrompt: 'When a report is submitted, send the approver a direct message.'}.Console snippet that swaps the GenerateRule answer locally
Offline tests
QA Steps
Staging answers
impossibleonly after Web-Expensify#56886. Until then, check that saving still works:PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari