Conversation
rhan-oai
force-pushed
the
pr16639
branch
2 times, most recently
from
f7d1f45 to
37390db
Compare
| impl InitialHistory { | ||
| pub fn scan_rollout_items(&self, mut predicate: impl FnMut(&RolloutItem) -> bool) -> bool { | ||
| match self { | ||
| InitialHistory::New => false, |
There was a problem hiding this comment.
determining is_first_turn for resumed or forked threads is tricky without explicit metadata, predicate looks for first user turn so performance cost should be negligible on average
rhan-oai
force-pushed
the
pr16639
branch
15 times, most recently
from
82caee0 to
d3d94c3
Compare
rhan-oai
force-pushed
the
pr16639
branch
10 times, most recently
from
9c63a0c to
f9dea6f
Compare
--- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/16638). * #16870 * #16706 * #16659 * #16641 * #16640 * __->__ #16638
rhan-oai
force-pushed
the
pr16639
branch
2 times, most recently
from
f85b185 to
3328a11
Compare
--- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/16638). * openai#16870 * openai#16706 * openai#16659 * openai#16641 * openai#16640 * __->__ openai#16638
codex-rs/core/src/thread_manager.rs
Outdated
| ) | ||
| } | ||
|
|
||
| pub fn new_with_analytics_events_client( |
There was a problem hiding this comment.
codex tends to do this a lot.... instead of defining a new function with a new parameter, can we just update new + the callsites? otherwise we will have function sprawl every time a new param is added
| last_agent_message | ||
| } | ||
|
|
||
| async fn track_turn_resolved_config_analytics( |
There was a problem hiding this comment.
should this function no-op if features.general_analytics is not enabled?
| fn collaboration_mode_mode(mode: ModeKind) -> &'static str { | ||
| match mode { | ||
| ModeKind::Plan => "plan", | ||
| ModeKind::Default | ModeKind::PairProgramming | ModeKind::Execute => "default", |
There was a problem hiding this comment.
hmm why do we coerce PairProgramming and Execute to "default"?
There was a problem hiding this comment.
both serialize to default as per protocol/src/config_types.rs
pub enum ModeKind {
Plan,
#[default]
#[serde(
alias = "code",
alias = "pair_programming",
alias = "execute",
alias = "custom"
)]
Default,
#[doc(hidden)]
#[serde(skip_serializing, skip_deserializing)]
#[schemars(skip)]
#[ts(skip)]
PairProgramming,
#[doc(hidden)]
#[serde(skip_serializing, skip_deserializing)]
#[schemars(skip)]
#[ts(skip)]
Execute,
}
pub const TUI_VISIBLE_COLLABORATION_MODES: [ModeKind; 2] = [ModeKind::Default, ModeKind::Plan];
There was a problem hiding this comment.
oh interesting. ok ignore me
codex-rs/core/src/codex.rs
Outdated
| conversation_history.scan_rollout_items(rollout_item_is_user_turn_boundary) | ||
| } | ||
|
|
||
| fn rollout_item_is_user_turn_boundary(item: &RolloutItem) -> bool { |
There was a problem hiding this comment.
nit: can we move these two functions to codex-rs/core/src/thread_rollout_truncation.rs?
can probably have something like:
// codex-rs/core/src/thread_rollout_truncation.rs
pub(crate) fn initial_history_has_user_turns(history: &InitialHistory) -> bool {
match history {
InitialHistory::New => false,
InitialHistory::Resumed(resumed) => rollout_has_user_turns(&resumed.history),
InitialHistory::Forked(items) => rollout_has_user_turns(items),
}
}
pub(crate) fn rollout_has_user_turns(items: &[RolloutItem]) -> bool {
!user_turn_positions_in_rollout(items).is_empty()
}
owenlin0
left a comment
owenlin0
left a comment
There was a problem hiding this comment.
Optional follow-up re: tests - we could probably move the failed/interrupted turn analytics permutations into the reducer tests and keep just one app-server integration test for the end-to-end turn event. would keep the app-server integration tests a bit more focused.
rhan-oai
force-pushed
the
pr16639
branch
9 times, most recently
from
720b401 to
19caf41
Compare
2 participants
Stack created with Sapling. Best reviewed with ReviewStack.